0 bytes/s vs. ∞ bytes/s

[積丹尼 Dan Jacobson]

Receiving objects: 100% (1003/1003), 1.15 MiB | 0 bytes/s, done.
Receiving objects: 100% (1861/1861), 11.74 MiB | 4.58 MiB/s, done.
Receiving objects: 100% (474/474), 160.72 KiB | 0 bytes/s, done.
Receiving objects: 100% (7190/7190), 26.02 MiB | 6.53 MiB/s, done.

If the connection is too fast to calculate, please report
∞ bytes/s or
inf bytes/s or
? bytes/s or
anything but 0 bytes/s, which means nothing (transmitted.)

bug with check-ref-format outside of repository

[Marko Kungla]

Hi,

As contrived e.g: if I have in my "workspace" sub directories mostly
git repositories, but some not and if I exec,
"for d in */ ; do (cd $d && git check-ref-format --branch @{-1});
done" then I get 3 possible responses.

git version: 2.13.0
1. Valid branch name
2. fatal: '@{-1}' is not a valid branch name
3. fatal: BUG: setup_git_env called without repository

git version 2.13.2.915.ga9c46e097
1. Valid branch name
2. fatal: '@{-1}' is not a valid branch name
3. BUG: environment.c:178: git environment hasn't been setup

best marko

Re: [RFC PATCH] t: lib-gpg: flush agent sockets on startup

[Santiago Torres]

Hello all,

I don't know if this is a desired feature, but I noticed that, one some
versions of gpg, gpg tests are skipped when I run a test suite multiple
times.

Cheers!
-Santiago.

On Fri, Jul 07, 2017 at 06:01:59PM -0400, santi...@nyu.edu wrote:
> From: Santiago Torres 
> 
> When running gpg-relevant tests, a gpg-daemon is ran for a
> trash_directory-specific GNUPGHOME. This daemon creates a unix socket on
> the target host, and it will be used on subsequent runs of the same test
> script.  Add a call to kill the agent and flush the sockets of the
> relevant trash directory.
> 
> Signed-off-by: Santiago Torres 
> ---
>  t/lib-gpg.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> index ec2aa8f68..22ef2fa87 100755
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -31,6 +31,7 @@ then
>   chmod 0700 ./gpghome &&
>   GNUPGHOME="$(pwd)/gpghome" &&
>   export GNUPGHOME &&
> + gpgconf --kill gpg-agent &&
>   gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
>   "$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
>   gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \
> -- 
> 2.13.2
> 


signature.asc
Description: PGP signature

[RFC PATCH] t: lib-gpg: flush agent sockets on startup

[santiago]

From: Santiago Torres 

When running gpg-relevant tests, a gpg-daemon is ran for a
trash_directory-specific GNUPGHOME. This daemon creates a unix socket on
the target host, and it will be used on subsequent runs of the same test
script.  Add a call to kill the agent and flush the sockets of the
relevant trash directory.

Signed-off-by: Santiago Torres 
---
 t/lib-gpg.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index ec2aa8f68..22ef2fa87 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -31,6 +31,7 @@ then
chmod 0700 ./gpghome &&
GNUPGHOME="$(pwd)/gpghome" &&
export GNUPGHOME &&
+   gpgconf --kill gpg-agent &&
gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \
-- 
2.13.2

Re: bug during checkout of remote branch and uncommited changes ?

[Luc Van Oostenryck]

On Fri, Jul 7, 2017 at 11:22 PM, Junio C Hamano  wrote:
> Luc Van Oostenryck  writes:
>
>> At least here, the scenario I gave allow to fully reproduce the problem.
>>
>> Would you like any other information?
>
> Not really.  Here is what I locally ran and its output
>
> -- >8 -- cut here -- >8 --
>
> #!/bin/sh
>
> mkdir -p /var/tmp/x/lvo && cd /var/tmp/x/lvo || exit
>
> rm -fr src dst
>
> (
> mkdir src &&
> cd src &&
> git init &&
> >file &&
> git add file &&
> git commit -m 'initial' &&
> git checkout -b abranch &&
> echo abranch >file &&
> git commit -a -m 'abranch'
> ) || exit
>
> mkdir dst &&
> cd dst &&
> git init &&
> git pull ../src master &&
> echo mine >file &&
> git status -suno &&
> git remote add aremote ../src &&
> git fetch aremote abranch || exit
>
> git checkout abranch
>
> git reset --hard
>
> git checkout abranch
>
> -- 8< -- cut here -- 8< --
>
> $ sh script
> Initialized empty Git repository in /var/tmp/x/lvo/src/.git/
> [master (root-commit) 8535bd5] initial
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  create mode 100644 file
> Switched to a new branch 'abranch'
> [abranch 32814d9] abranch
>  1 file changed, 1 insertion(+)
> Initialized empty Git repository in /var/tmp/x/lvo/dst/.git/
> From ../src
>  * branchmaster -> FETCH_HEAD
>  M file
> From ../src
>  * branchabranch-> FETCH_HEAD
>  * [new branch]  abranch-> aremote/abranch
> error: Your local changes to the following files would be overwritten by 
> checkout:
> file
> Please commit your changes or stash them before you switch branches.
> Aborting
> HEAD is now at 8535bd5 initial
> Switched to a new branch 'abranch'
> Branch abranch set up to track remote branch abranch from aremote.
>
> 
>
> As far as I can see, everything is working as intended.  The first
> "git checkout abranch" stops due to the dirty local state before it
> even tries to DWIM to create abranch, and then after resetting to
> get rid of all the local modifications, the second "git checkout
> abranch" manages to create the branch just fine.
>
> Even with different variations (like making dst a clone of src to
> force the "checkout" to see an ambiguity), I do not seem to get your
> "the first one fails with an error message about Ambiguity, the
> second does not", which indeed is a curious symptom.
>
> Without knowing if this is an ancient Git, or what remote other than
> aremote this repository has, what remote-tracking branches from
> these remotes this repository has, if they share the same name
> abranch that points at the same or different objects, etc., I simply
> do not know what is causing you the symptom, and time I allocated to
> look into this for now just ran out.

Strange.
I forgot to say that I was using git 2.13.0

I have a few more remote but 'abranch' was just a new branch (with
name unique to the remote) with fresh objects.

I'll investigate things more here and report if anything new.
Thanks, for your time.

-- Luc

Re: bug during checkout of remote branch and uncommited changes ?

[Junio C Hamano]

Luc Van Oostenryck  writes:

> At least here, the scenario I gave allow to fully reproduce the problem.
>
> Would you like any other information?

Not really.  Here is what I locally ran and its output

-- >8 -- cut here -- >8 --

#!/bin/sh

mkdir -p /var/tmp/x/lvo && cd /var/tmp/x/lvo || exit

rm -fr src dst

(
mkdir src &&
cd src &&
git init &&
>file &&
git add file &&
git commit -m 'initial' &&
git checkout -b abranch &&
echo abranch >file &&
git commit -a -m 'abranch'
) || exit

mkdir dst &&
cd dst &&
git init &&
git pull ../src master &&
echo mine >file &&
git status -suno &&
git remote add aremote ../src &&
git fetch aremote abranch || exit

git checkout abranch

git reset --hard

git checkout abranch

-- 8< -- cut here -- 8< --

$ sh script
Initialized empty Git repository in /var/tmp/x/lvo/src/.git/
[master (root-commit) 8535bd5] initial
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 file
Switched to a new branch 'abranch'
[abranch 32814d9] abranch
 1 file changed, 1 insertion(+)
Initialized empty Git repository in /var/tmp/x/lvo/dst/.git/
>From ../src
 * branchmaster -> FETCH_HEAD
 M file
>From ../src
 * branchabranch-> FETCH_HEAD
 * [new branch]  abranch-> aremote/abranch
error: Your local changes to the following files would be overwritten by 
checkout:
file
Please commit your changes or stash them before you switch branches.
Aborting
HEAD is now at 8535bd5 initial
Switched to a new branch 'abranch'
Branch abranch set up to track remote branch abranch from aremote.



As far as I can see, everything is working as intended.  The first
"git checkout abranch" stops due to the dirty local state before it
even tries to DWIM to create abranch, and then after resetting to
get rid of all the local modifications, the second "git checkout
abranch" manages to create the branch just fine.

Even with different variations (like making dst a clone of src to
force the "checkout" to see an ambiguity), I do not seem to get your
"the first one fails with an error message about Ambiguity, the
second does not", which indeed is a curious symptom.

Without knowing if this is an ancient Git, or what remote other than
aremote this repository has, what remote-tracking branches from
these remotes this repository has, if they share the same name
abranch that points at the same or different objects, etc., I simply
do not know what is causing you the symptom, and time I allocated to
look into this for now just ran out.

Re: [PATCH 11/12] sha1_name: convert get_sha1* to get_oid*

[brian m. carlson]

On Wed, Jul 05, 2017 at 11:38:51AM -0700, Junio C Hamano wrote:
> Stefan Beller  writes:
> 
> >> @@ -636,7 +636,7 @@ static int get_sha1_basic(const char *str, int len, 
> >> unsigned char *sha1,
> >> int detached;
> >>
> >> if (interpret_nth_prior_checkout(str, len, ) > 0) {
> >> -   detached = (buf.len == 40 && 
> >> !get_sha1_hex(buf.buf, sha1));
> >> +   detached = (!get_oid_hex(buf.buf, oid));
> >
> > omitting the length check here?
> 
> Good eyes.  It probably should check with the possible oid lengths.

I'll reroll.  This may have been a bad conflict resolution on my part.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Dropping a merge from history -- rebase or filter-branch or ...?

[Martin Langhoff]

Hi git-folk!

long time no see! I'm trying to do one of those "actually, please
don't" things that turn out to be needed in the field.

I need to open our next "for release" development branch from our
master, but without a couple of disruptive feature branches, which
have been merged into master already. We develop in github, so I'll
call them Pull Requests (PRs) as gh does.

So I'd like to run a filter-branch or git-rebase --interactive
--preserve-merges that drops some PRs. Problem is, they don't work!

filter-branch --commit-filter is fantastic, and gives me all the
control I want... except that it will "skip the commit", but still use
the trees in the later commits, so the code changes brought in by
those commits I wanted to avoid will be there. I think the docs/help
that discuss  "skip commit" should have a big warning there!

rebase --interactive --preserve-merges  --keep-empty made a complete
hash of things. Nonsense conflicts all over on the merge commits; I
think it re-ran the merge without picking up the conflict resolutions
we had applied.

The changes we want to avoid are fairly localized -- a specific module
got refactored in 3 stages. The rest of the history should replay
cleanly. I don't want to delete the module.

My fallback is a manually constructed revert. While still an option, I
think it's better to have a clean stat without sizable feature-branch
reverts.

cheers,



m
-- 
 martin.langh...@gmail.com
 - ask interesting questions  ~  http://linkedin.com/in/martinlanghoff
 - don't be distracted~  http://github.com/martin-langhoff
   by shiny stuff

Donation

[Clay Nina]

Hello, My name is Gloria C. Mackenzie, i have a Monetary Donation to make for 
the less privilege, yourself and your organization, am writing you with a 
friend's email, please contact me on mackenzie...@rogers.com

Re: [PATCH v2 0/4] reflog-walk fixes for maint

[Junio C Hamano]

Thanks.  All made sense to me after reading them twice.

Re: Why does git-checkout accept a tree-ish?

[Junio C Hamano]

Dan Fabulich  writes:

> Prior to this commit, git-checkout would only switch branches; you
> could use git-checkout-index to copy files from the index to the
> working tree. But in this commit, git-checkout not only subsumes
> the functionality of git-checkout-index but also learns the
> ability to copy files from an arbitrary branch (now an arbitrary
> tree-ish) into the working copy *and* the the index. (That was
> important because git-reset didn't accept  in 2005.)
> ...
> P.S. I would make a similar argument about adding  support
> to git-reset, rather than creating a separate command like
> git-unadd.

I do not have a habit of crying over spilt milk for too long, but if
we did not have "git reset ..." and adding an equivalent
feature today, I would guess that we would not have done it as a
different mode for "git reset".  We probably wouldn't have added an
extra command "unadd", either.

"git checkout --cached [] ..." would have been more
in line with the CLI convention for a command that can act on the
index and/or the working tree files ("--cached" is the way to make a
request to act only on the index without affecting the working tree
files).

As to why "git checkout" started taking , the thread referred
to in my previous response seems to give a better story than I could
remember ;-).  We were unhappy with checkout-index and the
suggestion that was first made publicly reused "git checkout".
While I initially showed my reluctance by calling the hypothetical
command "xx" instead of Linus's suggested overloading of
"checkout" in my response in the thread, I think the end-result
turned out to be not too bad, given that our users understand the
difference between 

 - checking out a branch to work on advancing the history of the
   branch; and

 - checking out a set of paths out of a  to modify the
   working tree contents while working on advancing the history of
   the current branch.

just fine.  When doing the former, you would of course want to
preserve local modifications in your working tree.  When doing the
latter, you are asking to overwrite the named paths (checking them
out of a tree-ish or the index is often done to wipe the mistaken
attempt to modify it and giving you a clean slate).

RE: [PATCH v5 7/7] fsmonitor: add a performance test

[David Turner]

> -Original Message-
> From: Junio C Hamano [mailto:jch2...@gmail.com] On Behalf Of Junio C
> Hamano
> Sent: Friday, July 7, 2017 2:35 PM
> To: Ben Peart 
> Cc: git@vger.kernel.org; benpe...@microsoft.com; pclo...@gmail.com;
> johannes.schinde...@gmx.de; David Turner ;
> p...@peff.net; christian.cou...@gmail.com; ava...@gmail.com
> Subject: Re: [PATCH v5 7/7] fsmonitor: add a performance test
> 
> Ben Peart  writes:
> 
> > On 6/14/2017 2:36 PM, Junio C Hamano wrote:
> >> Ben Peart  writes:
> >>
>  Having said all that, I think you are using this ONLY on windows;
>  perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of
>  the above and arrange Makefile to build test-drop-cache only on
>  that platform, or something?
> >>>
> >>> I didn't find any other examples of Windows only tools.  I'll update
> >>> the #ifdef to properly dump the file system cache on Linux as well
> >>> and only error out on other platforms.
> >>
> >> If this will become Windows-only, then I have no problem with
> >> platform specfic typedef ;-) I have no problem with CamelCase,
> >> either, as that follows the local convention on the platform (similar
> >> to those in compat/* that are only for Windows).
> >>
> >> Having said all that.
> >>
> >> Another approach is to build this helper on all platforms, ...
> 
> ... and having said all that, I think it is perfectly fine to do such a 
> clean-up long
> after the series gets more exposure to wider audiences as a follow-up patch.
> Let's get the primary part that affects people's everyday use of Git right 
> and then
> worry about the test details later.
> 
> A quick show of hands to the list audiences.  How many of you guys actually
> tried this series on 'pu' and checked to see its performance (and correctness 
> ;-)
> characteristics?
> 
> Do you folks like it?  Rather not have such complexity in the core part of the
> system?  A good first step to start adding more performance improvements?  No
> opinion?

I have not had the chance to test the latest version out yet.  The idea, 
broadly, seems sound, but as Ben notes in a later mail, the details are 
important.  Since he's going to re-roll with more interesting invalidation 
logic, I'll wait to try it again until a new version is available.

What's cooking in git.git (Jul 2017, #02; Fri, 7)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* ab/sha1dc-maint (2017-07-03) 1 commit
  (merged to 'next' on 2017-07-05 at ac69c90b7e)
 + sha1dc: update from upstream
 (this branch is used by ab/sha1dc.)

 Update the sha1dc again to fix portability glitches.


* ab/strbuf-addftime-tzname-boolify (2017-07-01) 2 commits
  (merged to 'next' on 2017-07-05 at 81e6795eb3)
 + strbuf: change an always NULL/"" strbuf_addftime() param to bool
 + strbuf.h comment: discuss strbuf_addftime() arguments in order

 strbuf_addftime() is further getting tweaked.


* aw/contrib-subtree-doc-asciidoctor (2017-06-27) 1 commit
  (merged to 'next' on 2017-06-30 at af23bd111b)
 + subtree: honour USE_ASCIIDOCTOR when set

 The Makefile rule in contrib/subtree for building documentation
 learned to honour USE_ASCIIDOCTOR just like the main documentation
 set does.


* jc/utf8-fprintf (2017-06-28) 1 commit
  (merged to 'next' on 2017-06-30 at a8cc490818)
 + submodule--helper: do not call utf8_fprintf() unnecessarily

 Code cleanup.


* js/fsck-name-object (2017-06-28) 1 commit
  (merged to 'next' on 2017-06-30 at 9a08514cf2)
 + t1450: use egrep for regexp "alternation"

 Test fix.


* js/t5534-rev-parse-gives-multi-line-output-fix (2017-07-05) 1 commit
  (merged to 'next' on 2017-07-05 at 5f964c44ba)
 + t5534: fix misleading grep invocation

 A few tests that tried to verify the contents of push certificates
 did not use 'git rev-parse' to formulate the line to look for in
 the certificate correctly.


* rs/apply-avoid-over-reading (2017-07-01) 1 commit
  (merged to 'next' on 2017-07-05 at 35730f3a47)
 + apply: use starts_with() in gitdiff_verify_name()

 Code clean-up to fix possible buffer over-reading.


* sb/merge-recursive-code-cleanup (2017-06-30) 1 commit
  (merged to 'next' on 2017-07-05 at 4228240520)
 + merge-recursive: use DIFF_XDL_SET macro

 Code clean-up.


* xz/send-email-batch-size (2017-07-05) 1 commit
  (merged to 'next' on 2017-07-05 at 92f3c31fbd)
 + send-email: --batch-size to work around some SMTP server limit

 "git send-email" learned to overcome some SMTP server limitation
 that does not allow many pieces of e-mails to be sent over a single
 session.

--
[New Topics]

* jc/allow-lazy-cas (2017-07-06) 1 commit
 - push: disable lazy --force-with-lease by default

 Because "git push --force-with-lease[=]" that relies on the
 stability of remote-tracking branches is unsafe when something
 fetches into the repository behind user's back, it is now disabled
 by default.  A new configuration variable can be used to enable it
 by users who know what they are doing.  This would pave the way to
 possibly turn `--force` into `--force-with-lease`.

 Will wait for feedback, then merge to and cook in 'next'.


* ks/typofix-commit-c-comment (2017-07-06) 1 commit
  (merged to 'next' on 2017-07-07 at 64e98fc832)
 + builtin/commit.c: fix a typo in the comment

 Typofix.

 Will merge to 'master'.


* bb/unicode-10.0 (2017-07-07) 1 commit
  (merged to 'next' on 2017-07-07 at a9c46e097b)
 + unicode: update the width tables to Unicode 10

 Update the character width tables.

 Will merge to 'master'.

--
[Stalled]

* mg/status-in-progress-info (2017-05-10) 2 commits
 - status --short --inprogress: spell it as --in-progress
 - status: show in-progress info for short status

 "git status" learns an option to report various operations
 (e.g. "merging") that the user is in the middle of.

 cf. 


* nd/worktree-move (2017-04-20) 6 commits
 - worktree remove: new command
 - worktree move: refuse to move worktrees with submodules
 - worktree move: accept destination as directory
 - worktree move: new command
 - worktree.c: add update_worktree_location()
 - worktree.c: add validate_worktree()

 "git worktree" learned move and remove subcommands.

 Expecting a reroll.
 cf. <20170420101024.7593-1-pclo...@gmail.com>
 cf. <20170421145916.mknekgqzhxffu...@sigill.intra.peff.net>
 cf. 


* sg/clone-refspec-from-command-line-config (2017-06-16) 2 commits
 - Documentation/clone: document ignored configuration variables
 - clone: respect additional configured fetch refspecs during initial fetch
 (this branch is used by sg/remote-no-string-refspecs.)

 "git clone -c var=val" is a way to set configuration variables in
 the resulting repository, but it is more useful to also make these
 variables take 

Re: [PATCH v5 7/7] fsmonitor: add a performance test

[Ben Peart]

On 7/7/2017 2:35 PM, Junio C Hamano wrote:

Ben Peart  writes:


On 6/14/2017 2:36 PM, Junio C Hamano wrote:

Ben Peart  writes:


Having said all that, I think you are using this ONLY on windows;
perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of
the above and arrange Makefile to build test-drop-cache only on that
platform, or something?


I didn't find any other examples of Windows only tools.  I'll update
the #ifdef to properly dump the file system cache on Linux as well and
only error out on other platforms.


If this will become Windows-only, then I have no problem with
platform specfic typedef ;-) I have no problem with CamelCase,
either, as that follows the local convention on the platform
(similar to those in compat/* that are only for Windows).

Having said all that.

Another approach is to build this helper on all platforms, ...


... and having said all that, I think it is perfectly fine to do
such a clean-up long after the series gets more exposure to wider
audiences as a follow-up patch.  Let's get the primary part that
affects people's everyday use of Git right and then worry about the
test details later.

A quick show of hands to the list audiences.  How many of you guys
actually tried this series on 'pu' and checked to see its
performance (and correctness ;-) characteristics?


TLDR: the current version isn't correct.

One of the things I did was hack up the test script to enable running 
all the tests with fsmonitor enabled.  I found a number of bugs in 
Watchman on Windows and have been working with Wez to get them fixed.


Just last week Watchman got to the point where I could run the complete 
git test suite.  As a result, I found that fsmonitor is overly 
aggressive in marking things with ce_mark_uptodate.  Submodules are 
currently broken as are commands that pass "--ignore-missing."


I started reworking the logic to fix these bugs and realized I was 
duplicating the set of tests that already exist in preload_thread. I'm 
currently working on integrating the logic into preload_thread so that 
both options can be used in combination and so I can avoid doing the 
(nearly identical) loop twice.




Do you folks like it?  Rather not have such complexity in the core
part of the system?  A good first step to start adding more
performance improvements?  No opinion?

Re: [PATCH v5 4/7] fsmonitor: add test cases for fsmonitor extension

[Ben Peart]

On 6/27/2017 12:20 PM, Christian Couder wrote:

On Sat, Jun 10, 2017 at 3:40 PM, Ben Peart  wrote:


+# fsmonitor works correctly with or without the untracked cache
+# but if it is available, we'll turn it on to ensure we test that
+# codepath as well.
+
+test_lazy_prereq UNTRACKED_CACHE '
+   { git update-index --test-untracked-cache; ret=$?; } &&
+   test $ret -ne 1
+'
+
+if test_have_prereq UNTRACKED_CACHE; then
+   git config core.untrackedcache true
+else
+   git config core.untrackedcache false
+fi


I wonder if it would be better to just do something like:


That is a good idea; I'll add that around the tests that aren't 
explicitly testing interop with and without the untracked cache.


Thanks!



=

test_expect_success 'setup' '
 
'

uc_values="false"
test_have_prereq UNTRACKED_CACHE && uc_values="false true"

for uc_val in $uc_values
do

 test_expect_success "setup untracked cache to $uc_val" '
  git config core.untrackedcache $uc_val
 '

 test_expect_success 'refresh_index() invalidates fsmonitor cache' '
   ...
 '

 test_expect_success "status doesn't detect unreported modifications" '
   ...
 '

...

done

=

Re: [PATCH v5 7/7] fsmonitor: add a performance test

[Junio C Hamano]

Ben Peart  writes:

> On 6/14/2017 2:36 PM, Junio C Hamano wrote:
>> Ben Peart  writes:
>>
 Having said all that, I think you are using this ONLY on windows;
 perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of
 the above and arrange Makefile to build test-drop-cache only on that
 platform, or something?
>>>
>>> I didn't find any other examples of Windows only tools.  I'll update
>>> the #ifdef to properly dump the file system cache on Linux as well and
>>> only error out on other platforms.
>>
>> If this will become Windows-only, then I have no problem with
>> platform specfic typedef ;-) I have no problem with CamelCase,
>> either, as that follows the local convention on the platform
>> (similar to those in compat/* that are only for Windows).
>>
>> Having said all that.
>>
>> Another approach is to build this helper on all platforms, ...

... and having said all that, I think it is perfectly fine to do
such a clean-up long after the series gets more exposure to wider
audiences as a follow-up patch.  Let's get the primary part that
affects people's everyday use of Git right and then worry about the
test details later.

A quick show of hands to the list audiences.  How many of you guys
actually tried this series on 'pu' and checked to see its
performance (and correctness ;-) characteristics?  

Do you folks like it?  Rather not have such complexity in the core
part of the system?  A good first step to start adding more
performance improvements?  No opinion?

Re: [PATCH 1/2] hooks: replace irrelevant hook sample

[Junio C Hamano]

Kaartic Sivaraam  writes:

> The pre-commit-msg hook sample has an example that comments
> the "Conflicts:" part of the merge commmit. It isn't relevant
> anymore as it's done by default since 261f315b ("merge & sequencer:
> turn "Conflicts:" hint into a comment", 2014-08-28).
>
> Add an alternative example that replaces it. This ensures there's
> at the least a simple example that illustrates what could be done
> using the hook just by enabling it.
>
> Signed-off-by: Kaartic Sivaraam 
> ---
>  I have made the second patch depend on the first one to avoid
>  conflicts that may occur. Further, it was meaningful to join
>  the two patches as they would go together (or) not at all.

Whether the two samples these patches implement are useful ones or
not, the early part of 2/2 that starts using named variables instead
of positional ones is an improvement.  It makes it easier to see
what is going on.

Patch 2/2 as posted forgets to update some references to $1, $2 and
$3, both in the actual code and also commented out part, e.g.

sed -e ... "$COMMIT_MSG_FILE" >"$SED_OUTPUT_TEMP" && mv "$SED_OUTPUT_TEMP" 
"$1"
# case "$2,$3" in

In the first one, $COMMIT_MSG_FILE and $1 refer to the same thing;
they should be consistent.

If these updates were to be done as multiple patches, the change to
use named not positional variables, without changing anything else,
should come second, I think, after the one that simply removes the
now-useless "Conflicts:" sample from the script, as a preparatory
clean-up step.  You can build other things like hints-removal and
sign-off with interpret-trailers on top of these two steps.

Have you considered using "@PERL_PATH@ -i" instead of "sed" in this
step, by the way?  That would allow you not to worry about the
temporary file left behind (e.g. what happens when somebody else
runs this in a shared repository setting, creating and leaving the
temporary file that you may not be able to write into later because
her umask is tighter, and then you try to make a commit).

Thanks.

>  templates/hooks--prepare-commit-msg.sample | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/templates/hooks--prepare-commit-msg.sample 
> b/templates/hooks--prepare-commit-msg.sample
> index 86b8f227e..5a638ebda 100755
> --- a/templates/hooks--prepare-commit-msg.sample
> +++ b/templates/hooks--prepare-commit-msg.sample
> @@ -9,8 +9,9 @@
>  #
>  # To enable this hook, rename this file to "prepare-commit-msg".
>  
> -# This hook includes three examples.  The first comments out the
> -# "Conflicts:" part of a merge commit.
> +# This hook includes three examples.  The first one removes three
> +# comment lines starting from the line that has the words
> +# "# Please enter the" in it's beginning.
>  #
>  # The second includes the output of "git diff --name-status -r"
>  # into the message, just before the "git status" output.  It is
> @@ -20,17 +21,16 @@
>  # The third example adds a Signed-off-by line to the message, that can
>  # still be edited.  This is rarely a good idea.
>  
> -case "$2,$3" in
> -  merge,)
> -@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; 
> print' "$1" ;;
> +sed -e '/^. Please enter the commit message /,/^#$/d' "$1" 
> >'.sed-output.temp' && mv '.sed-output.temp' "$1"
>  
> +# case "$2,$3" in
>  # ,|template,)
>  #   @PERL_PATH@ -i.bak -pe '
>  #  print "\n" . `git diff --cached --name-status -r`
>  # if /^#/ && $first++ == 0' "$1" ;;
> -
> -  *) ;;
> -esac
> +#
> +#  *) ;;
> +# esac
>  
>  # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: 
> \1/p')
>  # grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"

Re: [PATCH v5 7/7] fsmonitor: add a performance test

[Ben Peart]

On 6/14/2017 2:36 PM, Junio C Hamano wrote:

Ben Peart  writes:


Having said all that, I think you are using this ONLY on windows;
perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of
the above and arrange Makefile to build test-drop-cache only on that
platform, or something?


I didn't find any other examples of Windows only tools.  I'll update
the #ifdef to properly dump the file system cache on Linux as well and
only error out on other platforms.


If this will become Windows-only, then I have no problem with
platform specfic typedef ;-) I have no problem with CamelCase,
either, as that follows the local convention on the platform
(similar to those in compat/* that are only for Windows).

Having said all that.

Another approach is to build this helper on all platforms, with
sections protected by "#ifdef LINUX", "#ifdef WINDOWS", etc.  That
way, the platform detection and switching between running this
program and echoing something into /proc filesystem performed in
p7519 can be removed (this test-helper program will be responsible
to implement that logic instead).  When p7519 wants to drop the
filesystem cache, regardless of the platform, it can call this
test-helper without having to know how the filesystem cache is
dropped.



I'll take a cut at doing this but it is obviously very platform 
dependent and I'm unfamiliar with platforms other than Windows.


For everything other than Windows, my implementation will be calling 
"system" for external utilities based on what I can find on the web. 
Oh, and I have no way to test it other than on Windows so could use some 
review/testing from others. :)



I do not have strong preference either way, but I have a slight
suspicion that the "another approach" above may give us a better
result.  For one thing, the test-helper can be reused in new perf
scripts people will write in the future.

Thanks.

Re: Why does git-checkout accept a tree-ish?

[Junio C Hamano]

Dan Fabulich  writes:

> I was looking back through git's history, trying to figure out why
> git-checkout has so many features. I was struck by this commit by
> Junio in 2005.
>
> https://github.com/git/git/commit/4aaa702794447d9b281dd22fe532fd61e02434e1
>
>> git-checkout: revert specific paths to either index or a given tree-ish.
>> When extra paths arguments are given, git-checkout reverts only those
>> paths to either the version recorded in the index or the version
>> recorded in the given tree-ish.
>> 
>> This has been on the TODO list for quite a while.
>
> Prior to this commit, git-checkout would only switch branches; you
> could use git-checkout-index to copy files from the index to the
> working tree. But in this commit, git-checkout not only subsumes
> the functionality of git-checkout-index but also learns the
> ability to copy files from an arbitrary branch (now an arbitrary
> tree-ish) into the working copy *and* the the index. (That was
> important because git-reset didn't accept  in 2005.)
> ...
> And so I wonder if anybody knows just why git-checkout gained
> these two features in one commit, without creating a separate
> command.

The whole thread would explain it, I think.

https://public-inbox.org/git/pine.lnx.4.64.0510171814430.3...@g5.osdl.org/#t

Re: bug during checkout of remote branch and uncommited changes ?

[Luc Van Oostenryck]

On Fri, Jul 07, 2017 at 08:53:39AM -0700, Junio C Hamano wrote:
> Luc Van Oostenryck  writes:
> 
> > $ git reset --hard
> > patching file afile.c
> 
> Is that a message from something?  It does not sound like something
> "git reset --hard" would say.

Indeed, sorry. This is the result of a 'git diff | patch -p1 -R' to
which I'm used since a long time ago. I have no more reason to use
it but ... habits ... :)
 
But doing 'git reset --hard' gives exactly the same result.

> > $ git co 
> > fatal: Not tracking: ambiguous information for ref 
> > refs/remotes//
> >
> > What can be ambiguous here?
> 
> I think the message "Not tracking" is given when there is a remote
> other than  that also has .

Mmmm, no I don't have that.
At this point there is (in .git):
refs/remotes//
refs/heads/
The second one didn't existed before the checkout attempt, of course.

> Between the time you
> got the message and the time you tried to checkout , did
> anything happen to cause the second attempt succeed?

No.
At least here, the scenario I gave allow to fully reproduce the problem.

Would you like any other information?

-- Luc

Re: [PATCH v2 3/7] log: do not free parents when walking reflog

[Junio C Hamano]

Jeff King  writes:

> When we're doing a reflog walk (instead of walking the
> actual parent pointers), we may see commits multiple times.
> For this reason, we hold on to the commit buffer for each
> commit rather than freeing it after we've showed the commit.
>
> We should do the same for the parent list. Right now this is
> just a minor optimization. But once we refactor how reflog
> walks are performed, keeping the parents will avoid
> confusing us the second time we see the commit.
>
> Signed-off-by: Jeff King 
> ---
>  builtin/log.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 8ca1de9894..9c8bb3b5c3 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -374,9 +374,9 @@ static int cmd_log_walk(struct rev_info *rev)
>   if (!rev->reflog_info) {
>   /* we allow cycles in reflog ancestry */
>   free_commit_buffer(commit);
> + free_commit_list(commit->parents);
> + commit->parents = NULL;

After step 6/7, we no longer "allow cycles in reflog ancestry", as
there will be no reflog ancestry to speak of ;-), so it would be
nice to remove the comment above in that step.  But alternatively,
we can rephrase the comment here, to say something like "the same
commit can be shown multiple times while showing entries from the
reflog" instead.

>   }
> - free_commit_list(commit->parents);
> - commit->parents = NULL;
>   if (saved_nrl < rev->diffopt.needed_rename_limit)
>   saved_nrl = rev->diffopt.needed_rename_limit;
>   if (rev->diffopt.degraded_cc_to_c)

Re: [PATCH] unicode: update the width tables to Unicode 10

[Beat Bolli]

-- 
„It takes love over gold” — Dire Straits

> On 7 Jul 2017, at 17:43, Junio C Hamano  wrote:
> 
> Beat Bolli  writes:
> 
>> Now that the Unicode 10 has been announced[0], update the character
>> width tables to the new version.

Typo! Could you drop the first "the" from the message?

Thanks,
Beat

>> 
>> [0] 
>> http://blog.unicode.org/2017/06/announcing-unicode-standard-version-100.html
>> 
>> Signed-off-by: Beat Bolli 
>> ---
> 
> Thanks, again, for keeping an eye on the progress in the external
> world ;-)  Will apply.

Re: [RFC/WIP PATCH] object store classification

[Junio C Hamano]

Ben Peart  writes:

> For more API/state design purity, I wonder if there should be an
> object_store structure that is passed to each of the object store APIs
> instead of passing the repository object. The repository object could
> then contain an instance of the object_store structure.
>
> That said, I haven't take a close look at all the code in object.c to
> see if all the data needed can be cleanly abstracted into an
> object_store structure.

My gut feeling was it is just the large hashtable that keeps track of
objects we have seen, but the object replacement/grafts and other
things may also want to become per-repository.

> One concern I have is that the global state refactoring effort will
> just result in all the global state getting moved into a single
> (global) repository object thus limiting it's usefulness.

I actually am not worried about it that much, and I say this with
the background of having done the same "grouping a set of global
state variables into a single structure and turning them into a
single default instance" for the_index.  Whether you like it or not,
the majority of operations do work on the default instance---that
was why the operations could live with just "a set of global state
variables" in the first place, and the convenience compatibility
macros that allow you to operate on the fields of the default
instance as if they were separate variables have been a huge
typesaver that also reduces the cognitive burden.  I'd expect that
the same will hold for the new "repository" and the "object_store"
abstractions.

Re: [PATCH] t5534: fix misleading grep invocation

[Junio C Hamano]

Johannes Schindelin  writes:

> Yes, there are grep versions that behave differently... how did you guess?
>
> I am in the middle of an extended investigation trying to assess how
> feasible it would be to use a native Win32 port of BusyBox (started by
> long-time Git contributor Nguyễn Thái Ngọc Duy) in Git for Windows to
> execute the many, many remaining Unix shell scripts that are a core part
> of Git (including crucial functionality such as bisect, rebase, stash and
> submodule, for which we suffer portability and performance problems).

I've long thought that BusyBox was primarily about size and not
about performance, but I can imagine that it would be a big win to
be able to run things like "mkdir" and "rm" without fork/exec, as it
is likely to be extermely more expensive than preparing to call and
actually making system calls mkdir(2), unlink(2), etc.

Interesting.  I learned a new thing today, but apparently that
FEATURE_SH_NOFORK was not a very new development.  I do not think
anybody is crazy enough to attempt making Git a nofork applet,
though ;-)

[PATCH 2/2] hooks: add signature using "interpret-trailers"

[Kaartic Sivaraam]

The sample hook to prepare the commit message before
a commit allows users to opt-in to add the signature
to the commit message. The signature is added at a place
that isn't consistent with the "-s" option of "git commit".
Further, it could go out of view in certain cases.

Add the signature in a way similar to "-s" option of
"git commit" using git's interpret-trailers command.

It works well in all cases except when the user invokes
"git commit" without any arguments. In that case manually
add a new line after the first line to ensure it's consistent
with the output of "-s" option.

While at it, name the input parameters to improve readability
of script.

Signed-off-by: Kaartic Sivaraam 
---
 templates/hooks--prepare-commit-msg.sample | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/templates/hooks--prepare-commit-msg.sample 
b/templates/hooks--prepare-commit-msg.sample
index 5a638ebda..7495078cb 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -21,7 +21,12 @@
 # The third example adds a Signed-off-by line to the message, that can
 # still be edited.  This is rarely a good idea.
 
-sed -e '/^. Please enter the commit message /,/^#$/d' "$1" >'.sed-output.temp' 
&& mv '.sed-output.temp' "$1"
+COMMIT_MSG_FILE=$1
+COMMIT_SOURCE=$2
+SHA1=$3
+
+SED_OUTPUT_TEMP='.sed-output-temp'
+sed -e '/^. Please enter the commit message /,/^#$/d' "$COMMIT_MSG_FILE" 
>"$SED_OUTPUT_TEMP" && mv "$SED_OUTPUT_TEMP" "$1"
 
 # case "$2,$3" in
 # ,|template,)
@@ -32,5 +37,14 @@ sed -e '/^. Please enter the commit message /,/^#$/d' "$1" 
>'.sed-output.temp' &
 #  *) ;;
 # esac
 
+# TEMP_FILE='.template-temp'
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
-# grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
+# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
+# if test -z "$COMMIT_SOURCE"
+# then
+#   {
+# echo
+# cat "$COMMIT_MSG_FILE"
+#   } >"$TEMP_FILE"
+#   mv "$TEMP_FILE" "$COMMIT_MSG_FILE"
+# fi
-- 
2.13.2.879.g2ab69f31a.dirty

[PATCH 1/2] hooks: replace irrelevant hook sample

[Kaartic Sivaraam]

The pre-commit-msg hook sample has an example that comments
the "Conflicts:" part of the merge commmit. It isn't relevant
anymore as it's done by default since 261f315b ("merge & sequencer:
turn "Conflicts:" hint into a comment", 2014-08-28).

Add an alternative example that replaces it. This ensures there's
at the least a simple example that illustrates what could be done
using the hook just by enabling it.

Signed-off-by: Kaartic Sivaraam 
---
 I have made the second patch depend on the first one to avoid
 conflicts that may occur. Further, it was meaningful to join
 the two patches as they would go together (or) not at all.

 templates/hooks--prepare-commit-msg.sample | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/templates/hooks--prepare-commit-msg.sample 
b/templates/hooks--prepare-commit-msg.sample
index 86b8f227e..5a638ebda 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -9,8 +9,9 @@
 #
 # To enable this hook, rename this file to "prepare-commit-msg".
 
-# This hook includes three examples.  The first comments out the
-# "Conflicts:" part of a merge commit.
+# This hook includes three examples.  The first one removes three
+# comment lines starting from the line that has the words
+# "# Please enter the" in it's beginning.
 #
 # The second includes the output of "git diff --name-status -r"
 # into the message, just before the "git status" output.  It is
@@ -20,17 +21,16 @@
 # The third example adds a Signed-off-by line to the message, that can
 # still be edited.  This is rarely a good idea.
 
-case "$2,$3" in
-  merge,)
-@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' 
"$1" ;;
+sed -e '/^. Please enter the commit message /,/^#$/d' "$1" >'.sed-output.temp' 
&& mv '.sed-output.temp' "$1"
 
+# case "$2,$3" in
 # ,|template,)
 #   @PERL_PATH@ -i.bak -pe '
 #  print "\n" . `git diff --cached --name-status -r`
 #   if /^#/ && $first++ == 0' "$1" ;;
-
-  *) ;;
-esac
+#
+#  *) ;;
+# esac
 
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
 # grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
-- 
2.13.2.879.g2ab69f31a.dirty

[PATCH] commit: fix a typo in the comment

[Kaartic Sivaraam]

Signed-off-by: Kaartic Sivaraam 
---
 builtin/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8d1cac062..aff6bf7aa 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -984,7 +984,7 @@ static int rest_is_empty(struct strbuf *sb, int start)
int i, eol;
const char *nl;
 
-   /* Check if the rest is just whitespace and Signed-of-by's. */
+   /* Check if the rest is just whitespace and Signed-off-by's. */
for (i = start; i < sb->len; i++) {
nl = memchr(sb->buf + i, '\n', sb->len - i);
if (nl)
-- 
2.13.2.879.g2ab69f31a.dirty

Re: [PATCH v2 6/7] reflog-walk: stop using fake parents

[Kyle Meyer]

Jeff King  writes:

>  Prior to this commit, we show both entries with
>  identical reflog messages. After this commit, we show
>  only the "comes back" entry. See the update in t3200
>  which demonstrates this.
>
>  Arguably either is fine, as the whole double-entry
>  thing is a bit hacky in the first place. And until a
>  recent fix, we truncated the traversal in such a case
>  anyway, which was _definitely_ wrong.

Yeah, I agree that the double-entry thing is a bit hacky and only
showing the "comes back" entry makes sense.

And with this change, I believe that the display of a rename event will
be the same for HEAD's log and the renamed branch's log, despite the
underlying entries having a different representation.

-- 
Kyle

Re: [PATCH v2 1/7] t1414: document some reflog-walk oddities

[Kyle Meyer]

Jeff King  writes:

> +test_expect_failure 'walking multiple reflogs shows all' '
> + # We expect to see all entries for all reflogs, but interleaved by
> + # date, with order no the command line breaking ties. We

s/order no/order on/

-- 
Kyle

Re: bug during checkout of remote branch and uncommited changes ?

[Junio C Hamano]

Luc Van Oostenryck  writes:

>   $ git reset --hard
>   patching file afile.c

Is that a message from something?  It does not sound like something
"git reset --hard" would say.

>   $ git co 
>   fatal: Not tracking: ambiguous information for ref 
> refs/remotes//
>
> What can be ambiguous here?

I think the message "Not tracking" is given when there is a remote
other than  that also has .  Between the time you
got the message and the time you tried to checkout , did
anything happen to cause the second attempt succeed?

> Strangely, trying a second time, succeed:
>   $ git co 
>   Previous HEAD position was ...
>   Switched to branch ''
>
> -- Luc Van Oostenryck

Re: [PATCH] unicode: update the width tables to Unicode 10

[Junio C Hamano]

Beat Bolli  writes:

> Now that the Unicode 10 has been announced[0], update the character
> width tables to the new version.
>
> [0] 
> http://blog.unicode.org/2017/06/announcing-unicode-standard-version-100.html
>
> Signed-off-by: Beat Bolli 
> ---

Thanks, again, for keeping an eye on the progress in the external
world ;-)  Will apply.

Re: [PATCH] hooks: replace irrelevant hook sample

[Kaartic Sivaraam]

On Fri, 2017-07-07 at 08:05 -0700, Junio C Hamano wrote:
> That is because I wear multiple hats, because I try to help in
> different ways, and because open source is not a battle to see whose
> idea is more right, but is a cooperative process to find a better
> solution together.
> 
Thanks for helping and being kind!

> As a fellow contributor, I do not think that removing the hint that
> is commented out, which is meant to be helpful to users while in
> their editor and which will be removed after the editor finishes
> anyway, is a useful enough example to keep the now otherwise useless
> sample hook.  But as the maintainer, I can see that you are still
> making sincere efforts to come up with a useful example and improve
> the end-user experience, and more importantly, I haven't heard from
> other people what they think---the only thing I have are different
> opinions from two people.  That is why I am not deciding and telling
> you to go find another area to hack in.
> 
> At the same time, I found that your implementation of the idea, i.e.
> removal of the commented-out hint, can be improved.  With an
> improved implementation of the proposed solution, it may have a
> better chance to be supported by others on the list, and equally
> importantly, if it turns out that other people do support what this
> patch tries to do, i.e. keep the sample hook alive by replacing the
> now useless examples with this one, we would have a better
> implementation of it.  And that is something I can help with, while
> I, the maintainer, is waiting.
> 
So, I'll improve it and of course wait for any replies. BTW, thanks for
clearing off the little confusion I had. I'm not used to these
multiple-hat replies from single person. Thanks for exposing me to it.

> Oh, by the way, what the maintainer is waiting for is not just "me
> too"s; this is not exactly a "having more people wins" democracy.
Got it.

-- 
Quote: “The most valuable person on any team is the person who makes
everyone else on the team more valuable”

Regards,
Kaartic

Re: [PATCH] push: disable lazy --force-with-lease by default

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> Which is why I think we should take Francesco's patch (with fixes from
> feedback), instead of Junio's.

The patch in this discussion is not meant as a replacement for the
one from Francesco.  It was meant as a companion patch.  

As I view the form of the option that relies on the stability of
remote-tracking branches strictly worse than the honest "--force"
that loudly advertises itself as dangerous (as opposed to being
advertised as a safer option, when it isn't), I consider the change
to require users to opt into relying on remote-tracking branches as
a prerequisite before we can recommend the form as a safer version
of "--force".

Re: [PATCH] hooks: replace irrelevant hook sample

[Junio C Hamano]

Kaartic Sivaraam  writes:

> That said, in case my interpretation that "'prepare-commit-msg' hook is
> not to be shipped due to it's uselessness" is correct, the reply of
> this mail as a whole seems to contradict it.

That is because I wear multiple hats, because I try to help in
different ways, and because open source is not a battle to see whose
idea is more right, but is a cooperative process to find a better
solution together.

As a fellow contributor, I do not think that removing the hint that
is commented out, which is meant to be helpful to users while in
their editor and which will be removed after the editor finishes
anyway, is a useful enough example to keep the now otherwise useless
sample hook.  But as the maintainer, I can see that you are still
making sincere efforts to come up with a useful example and improve
the end-user experience, and more importantly, I haven't heard from
other people what they think---the only thing I have are different
opinions from two people.  That is why I am not deciding and telling
you to go find another area to hack in.

At the same time, I found that your implementation of the idea, i.e.
removal of the commented-out hint, can be improved.  With an
improved implementation of the proposed solution, it may have a
better chance to be supported by others on the list, and equally
importantly, if it turns out that other people do support what this
patch tries to do, i.e. keep the sample hook alive by replacing the
now useless examples with this one, we would have a better
implementation of it.  And that is something I can help with, while
I, the maintainer, is waiting.

Oh, by the way, what the maintainer is waiting for is not just "me
too"s; this is not exactly a "having more people wins" democracy.

Re: [RFC/WIP PATCH] object store classification

[Ben Peart]

On 7/6/2017 10:33 PM, Junio C Hamano wrote:

Stefan Beller  writes:


Subject: Re: [RFC/WIP PATCH] object store classification


I thought you are writing different object-store backends and
classifying them into many categories (e.g. local, networked,
telepathic, etc.)

It is a logical next step to put per-repository things into
the_repository.  I skimmed the patch and the changes smelled sane,
but I didn't read it really carefully with fine toothed comb.

Thanks.  The remainder kept for reference, but no additional
comments in there.


This continues the efforts of bw/repo-object, making our code base
more object oriented by adding the object store to the the repository struct.



It's great to see this effort continuing.


Long term goal of the series that would evolve from this discussion:
* Easier to implement submodule features as it can be in the same process.

Short term goal:
* get rid of 'add_submodule_odb' in submodule.c
   When fetching or pushing we need to determine if a submodule needs processing
   by finding out if certain commits are present.  To do this we add the 
submodule
   objects as an alternate object database and do the processing in the same
   process. In case of multiple submodules, this would pollute the object store
   hence being slower and increasing memory usage, too.

This patch only changes the object store code to be object oriented based
on the repository struct.

To go for the short term goal we'd need to convert a few more places, mostly
all the construction (blob.c, tree.c, commit.c)



For more API/state design purity, I wonder if there should be an 
object_store structure that is passed to each of the object store APIs 
instead of passing the repository object. The repository object could 
then contain an instance of the object_store structure.


That said, I haven't take a close look at all the code in object.c to 
see if all the data needed can be cleanly abstracted into an 
object_store structure.


One concern I have is that the global state refactoring effort will just 
result in all the global state getting moved into a single (global) 
repository object thus limiting it's usefulness.


I'd also love to see this followed up with additional patches that would 
remove the back compat macros.  Should be a relatively straight forward 
"search and replace" series of patches.  That will eliminate the code 
readability/maintenance issues that comes along with macros in addition 
to the APIs themselves.


No additional comments below...


This is marked RFC as I'd want to gather feedback if the approach, presented
in the header files is sound.

Thanks!

Signed-off-by: Stefan Beller 
---

   Peff, this area of the code seems performance sensitive for a variety of
   use cases, specifically yours. So I cc'd you here. :)

  object.c | 77 +++-
  object.h | 51 
  repository.h |  6 +
  3 files changed, 92 insertions(+), 42 deletions(-)

diff --git a/object.c b/object.c
index 06ba3a11d8..b5ec0bb2f9 100644
--- a/object.c
+++ b/object.c
@@ -5,17 +5,15 @@
  #include "commit.h"
  #include "tag.h"
  
-static struct object **obj_hash;

-static int nr_objs, obj_hash_size;
-
-unsigned int get_max_object_index(void)
+unsigned int object_store_get_max_index(struct repository *r)
  {
-   return obj_hash_size;
+   return r->objects.obj_hash_size;
  }
  
-struct object *get_indexed_object(unsigned int idx)

+struct object *object_store_get_indexed(struct repository *r,
+   unsigned int idx)
  {
-   return obj_hash[idx];
+   return r->objects.obj_hash[idx];
  }
  
  static const char *object_type_strings[] = {

@@ -82,11 +80,15 @@ static void insert_obj_hash(struct object *obj, struct 
object **hash, unsigned i
   * Look up the record for the given sha1 in the hash map stored in
   * obj_hash.  Return NULL if it was not found.
   */
-struct object *lookup_object(const unsigned char *sha1)
+struct object *object_store_lookup(struct repository *r,
+  const unsigned char *sha1)
  {
unsigned int i, first;
struct object *obj;
  
+	struct object **obj_hash = r->objects.obj_hash;

+   int obj_hash_size = r->objects.obj_hash_size;
+
if (!obj_hash)
return NULL;
  
@@ -114,29 +116,31 @@ struct object *lookup_object(const unsigned char *sha1)

   * power of 2 (but at least 32).  Copy the existing values to the new
   * hash map.
   */
-static void grow_object_hash(void)
+static void grow_object_hash(struct repository *r)
  {
int i;
/*
 * Note that this size must always be power-of-2 to match hash_obj
 * above.
 */
-   int new_hash_size = obj_hash_size < 32 ? 32 : 2 * obj_hash_size;
+   int new_hash_size = r->objects.obj_hash_size < 32 ?
+   32 : 2 * 

bug during checkout of remote branch and uncommited changes ?

[Luc Van Oostenryck]

Hi,

Lately I've been caught several time with a problem which,
to me, is a bug. It can easily be reproduced, so I think
It's very possible it has already been reported.

The problem arise when trying to checkout a branch while having
some uncommited changes. The scenario is the following:
$ git status
M  afile.c
$ git remote add   
$ git fetch  
remote: Counting objects: 7, done.
remote: Compressing objects: 100% (6/6), done.
remote: Total 7 (delta 5), reused 3 (delta 1)
Unpacking objects: 100% (7/7), done.
From 
 * branch -> FETCH_HEAD
 * [new branch]   -> /
$ git co 
error: Your local changes to the following files would be overwritten 
by checkout:
afile.c
Please commit your changes or stash them before you switch branches.
Aborting
$ git reset --hard
patching file afile.c
$ git co 
fatal: Not tracking: ambiguous information for ref 
refs/remotes//

What can be ambiguous here?
Strangely, trying a second time, succeed:
$ git co 
Previous HEAD position was ...
Switched to branch ''

-- Luc Van Oostenryck

[PATCH] unicode: update the width tables to Unicode 10

[Beat Bolli]

Now that the Unicode 10 has been announced[0], update the character
width tables to the new version.

[0] http://blog.unicode.org/2017/06/announcing-unicode-standard-version-100.html

Signed-off-by: Beat Bolli 
---
 unicode_width.h | 42 +-
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/unicode_width.h b/unicode_width.h
index 02207be4f..6dee2c77c 100644
--- a/unicode_width.h
+++ b/unicode_width.h
@@ -51,6 +51,7 @@ static const struct interval zero_width[] = {
 { 0x0AC7, 0x0AC8 },
 { 0x0ACD, 0x0ACD },
 { 0x0AE2, 0x0AE3 },
+{ 0x0AFA, 0x0AFF },
 { 0x0B01, 0x0B01 },
 { 0x0B3C, 0x0B3C },
 { 0x0B3F, 0x0B3F },
@@ -73,7 +74,8 @@ static const struct interval zero_width[] = {
 { 0x0CC6, 0x0CC6 },
 { 0x0CCC, 0x0CCD },
 { 0x0CE2, 0x0CE3 },
-{ 0x0D01, 0x0D01 },
+{ 0x0D00, 0x0D01 },
+{ 0x0D3B, 0x0D3C },
 { 0x0D41, 0x0D44 },
 { 0x0D4D, 0x0D4D },
 { 0x0D62, 0x0D63 },
@@ -158,7 +160,7 @@ static const struct interval zero_width[] = {
 { 0x1CED, 0x1CED },
 { 0x1CF4, 0x1CF4 },
 { 0x1CF8, 0x1CF9 },
-{ 0x1DC0, 0x1DF5 },
+{ 0x1DC0, 0x1DF9 },
 { 0x1DFB, 0x1DFF },
 { 0x200B, 0x200F },
 { 0x202A, 0x202E },
@@ -262,6 +264,15 @@ static const struct interval zero_width[] = {
 { 0x1171D, 0x1171F },
 { 0x11722, 0x11725 },
 { 0x11727, 0x1172B },
+{ 0x11A01, 0x11A06 },
+{ 0x11A09, 0x11A0A },
+{ 0x11A33, 0x11A38 },
+{ 0x11A3B, 0x11A3E },
+{ 0x11A47, 0x11A47 },
+{ 0x11A51, 0x11A56 },
+{ 0x11A59, 0x11A5B },
+{ 0x11A8A, 0x11A96 },
+{ 0x11A98, 0x11A99 },
 { 0x11C30, 0x11C36 },
 { 0x11C38, 0x11C3D },
 { 0x11C3F, 0x11C3F },
@@ -269,6 +280,11 @@ static const struct interval zero_width[] = {
 { 0x11CAA, 0x11CB0 },
 { 0x11CB2, 0x11CB3 },
 { 0x11CB5, 0x11CB6 },
+{ 0x11D31, 0x11D36 },
+{ 0x11D3A, 0x11D3A },
+{ 0x11D3C, 0x11D3D },
+{ 0x11D3F, 0x11D45 },
+{ 0x11D47, 0x11D47 },
 { 0x16AF0, 0x16AF4 },
 { 0x16B30, 0x16B36 },
 { 0x16F8F, 0x16F92 },
@@ -339,7 +355,7 @@ static const struct interval double_width[] = {
 { 0x3000, 0x303E },
 { 0x3041, 0x3096 },
 { 0x3099, 0x30FF },
-{ 0x3105, 0x312D },
+{ 0x3105, 0x312E },
 { 0x3131, 0x318E },
 { 0x3190, 0x31BA },
 { 0x31C0, 0x31E3 },
@@ -358,10 +374,11 @@ static const struct interval double_width[] = {
 { 0xFE68, 0xFE6B },
 { 0xFF01, 0xFF60 },
 { 0xFFE0, 0xFFE6 },
-{ 0x16FE0, 0x16FE0 },
+{ 0x16FE0, 0x16FE1 },
 { 0x17000, 0x187EC },
 { 0x18800, 0x18AF2 },
-{ 0x1B000, 0x1B001 },
+{ 0x1B000, 0x1B11E },
+{ 0x1B170, 0x1B2FB },
 { 0x1F004, 0x1F004 },
 { 0x1F0CF, 0x1F0CF },
 { 0x1F18E, 0x1F18E },
@@ -370,6 +387,7 @@ static const struct interval double_width[] = {
 { 0x1F210, 0x1F23B },
 { 0x1F240, 0x1F248 },
 { 0x1F250, 0x1F251 },
+{ 0x1F260, 0x1F265 },
 { 0x1F300, 0x1F320 },
 { 0x1F32D, 0x1F335 },
 { 0x1F337, 0x1F37C },
@@ -392,15 +410,13 @@ static const struct interval double_width[] = {
 { 0x1F6CC, 0x1F6CC },
 { 0x1F6D0, 0x1F6D2 },
 { 0x1F6EB, 0x1F6EC },
-{ 0x1F6F4, 0x1F6F6 },
-{ 0x1F910, 0x1F91E },
-{ 0x1F920, 0x1F927 },
-{ 0x1F930, 0x1F930 },
-{ 0x1F933, 0x1F93E },
-{ 0x1F940, 0x1F94B },
-{ 0x1F950, 0x1F95E },
-{ 0x1F980, 0x1F991 },
+{ 0x1F6F4, 0x1F6F8 },
+{ 0x1F910, 0x1F93E },
+{ 0x1F940, 0x1F94C },
+{ 0x1F950, 0x1F96B },
+{ 0x1F980, 0x1F997 },
 { 0x1F9C0, 0x1F9C0 },
+{ 0x1F9D0, 0x1F9E6 },
 { 0x2, 0x2FFFD },
 { 0x3, 0x3FFFD }
 };
-- 
2.13.2.753.g7f5404b

Re: [PATCH] hooks: replace irrelevant hook sample

[Kaartic Sivaraam]

On Wed, 2017-07-05 at 12:50 -0700, Junio C Hamano wrote:
> Three things that caught my eyes:
> 
>  - Between "git commit --cleanup=strip" and "git commit --
> cleanup=verbatim",
>    lines that make up this initial instruction section are different.
> 
>  - "git grep 'Please enter the '" finds that this string is subject
>    to translation, so the pattern may not match (in which case it
>    will be a no-op without doing any harm, which is OK).
> 
>  - core.commentChar can be set to something other than '#', so the
>    pattern may not match (I do not offhand know if that may cause a
>    wrong line to match, causing harm, or not).
> 
> As merely an example, it probably is OK to say "this won't work if
> you are not using the C locale, and/or you are using custom
> core.commentChar".  So if we disregard the latter two, I would think
> 
> sed -e '/^# Please enter the commit message /,/^#$/d'
> 
> may be simpler to reason about to achieve the same goal.  
> 
Thanks for enlightening me about this. I thought sed was greedy with
address spaces the same way it's greedy with regex.

sed -e '/^# Please enter the commit message /,/^#$/d'


This command does seem to work regardless of the cleanup mode used.

That said, in case my interpretation that "'prepare-commit-msg' hook is
not to be shipped due to it's uselessness" is correct, the reply of
this mail as a whole seems to contradict it.

Should I work on this patch and another related one (he one that
modifies the signature part of the hook) or
should I drop it ?

IOW, would this patch likely make the hook useful again?

-- 
Kaartic

Re: [PATCH] t5534: fix misleading grep invocation

[Johannes Schindelin]

Hi Michael,

On Thu, 6 Jul 2017, Michael J Gruber wrote:

> Junio C Hamano venit, vidit, dixit 05.07.2017 18:26:
> > Johannes Schindelin  writes:
> > 
> >> It seems to be a little-known feature of `grep` (and it certainly came
> >> as a surprise to this here developer who believed to know the Unix tools
> >> pretty well) that multiple patterns can be passed in the same
> >> command-line argument simply by separating them by newlines. Watch, and
> >> learn:
> >>
> >>$ printf '1\n2\n3\n' | grep "$(printf '1\n3\n')"
> >>1
> >>3
> >>
> >> That behavior also extends to patterns passed via `-e`, and it is not
> >> modified by passing the option `-E` (but trying this with -P issues the
> >> error "grep: the -P option only supports a single pattern").
> >>
> >> It seems that there are more old Unix hands who are surprised by this
> >> behavior, as grep invocations of the form
> >>
> >>grep "$(git rev-parse A B) C" file
> >>
> >> were introduced in a85b377d041 (push: the beginning of "git push
> >> --signed", 2014-09-12), and later faithfully copy-edited in b9459019bbb
> >> (push: heed user.signingkey for signed pushes, 2014-10-22).
> >>
> >> Please note that the output of `git rev-parse A B` separates the object
> >> IDs via *newlines*, not via spaces, and those newlines are preserved
> >> because the interpolation is enclosed in double quotes.
> >>
> >> As a consequence, these tests try to validate that the file contains
> >> either A's object ID, or B's object ID followed by C, or both. Clearly,
> >> however, what the test wanted to see is that there is a line that
> >> contains all of them.
> >>
> >> This is clearly unintended, and the grep invocations in question really
> >> match too many lines.
>
> [...]
>
> How did you spot this? Are there grep versions that behave differently?

Yes, there are grep versions that behave differently... how did you guess?

I am in the middle of an extended investigation trying to assess how
feasible it would be to use a native Win32 port of BusyBox (started by
long-time Git contributor Nguyễn Thái Ngọc Duy) in Git for Windows to
execute the many, many remaining Unix shell scripts that are a core part
of Git (including crucial functionality such as bisect, rebase, stash and
submodule, for which we suffer portability and performance problems).

And it is BusyBox' grep that does not handle newlines in the pattern
argument to split it into two alternative patterns.

I first considered patching BusyBox to adhere to the expected behavior,
but then I looked closer and saw that the test's grep invocations actually
matched two lines instead of what I expected. An even closer look made me
suspect that the original intention was different from what the script
actually does, and for once I tried to be nice in my commit message.

Ciao,
Dscho

Re: [PATCH] push: disable lazy --force-with-lease by default

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 07 2017, Stefan Haller jotted:

> Junio C Hamano  wrote:
>
>> It turns out that some people use third-party tools that fetch from
>> remote and update the remote-tracking branches behind users' back,
>> defeating the safety relying on the stability of the remote-tracking
>> branches.
>
> Third-party tools are not the only problem. They may make the problem
> more likely to occur, but it can also happen without them. (See below.)
>
>> Let's disable the form that relies on the stability of remote-tracking
>> branches by default, and allow users who _know_ their remote-tracking
>> branches are stable to enable it with a configuration variable.
>
> I'm wondering if people who claim they know they are safe really do.
> Elsewhere in the other thread somebody said "I only ever explicitly
> fetch, so I know I'm safe". Are you sure?
>
> Consider this example:

Both of your examples explicitly fetch. Yes this could be confusing to
someone who doesn't understand that "git fetch" doesn't just fetch the
current remote branch, but all branches.

> What I'm getting at is that there's a lot of things that you have to
> remember to not do in order to make --force-with-lease without parameter
> a useful tool.

Fully agreed, it's confusing, but it's less shitty than --force.

The concern I have with Junio's patch above (but I like Francesco
Mazzoli's approach better) is that the safety of the various --force
options, from least safe to most safe, is:

 1. --force: You blow away the remote history, no idea what's there, or
if your local ref mirrors what you just wiped.

 2. --force-with-lease: Even if you have a `git fetch` in the
 background, at least if you wipe a remote ref you have a copy in a
 local reflog to restore it.

 3. --force-with-lease=master:origin/master: More explicit, but still
 subject to the caveat with background fetching.

 4. --force-with-lease=master:: You know exactly
 what you're wiping, and have likely reviewed that exact commit.

Yes, #4 is the safest, #2 & #3 are similar but subject to various
caveats with background fetching / users not realizing "git pull"
fetches everything etc.

But I think we have to keep our eye on the ball here. Which is to enact
a net increase in user safety.

Right now most users who want to force a remote branch just use
--force. E.g. Stack Overflow shows >100k results for git + --force, but
just 500 for git + --force-with-lease.

You and others are rightly pointing out that --force-with-lease has lots
of caveats, but that as an argument-less flag is something we could
(with Francesco patch) turn on by default as a --force replacement.

This would leave users better off than they were before, because now
when they accidentally wipe something they at least have a local copy if
they did the wrong thing.

Moving everyone from #1 to #2 would be a net increase in user safety
without more complex UX. Not having #2 would, for a lot of users who'd
otherwise be happy to use #2, mean they'll just use #1 (the least safe
option!) instead of the more ideal #4.

Which is why I think we should take Francesco's patch (with fixes from
feedback), instead of Junio's.

Re: [PATCH] push: disable lazy --force-with-lease by default

[Jeff King]

On Fri, Jul 07, 2017 at 11:24:15AM +0200, Stefan Haller wrote:

> > Let's disable the form that relies on the stability of remote-tracking
> > branches by default, and allow users who _know_ their remote-tracking
> > branches are stable to enable it with a configuration variable.
> 
> I'm wondering if people who claim they know they are safe really do.
> Elsewhere in the other thread somebody said "I only ever explicitly
> fetch, so I know I'm safe". Are you sure?
> 
> Consider this example:

Thanks, these are all really good examples.

I think another one is just:

  $ git fetch
  [time passes]
  $ git checkout branch
  $ git rebase -i
  [oops, I forgot to merge in the latest changes before rewriting]
  $ git push --force-with-lease

That doesn't even require a fetch/pull after you start working. It's
simply a mismatch between reality and what the default assumes (that
whatever you were working on incorporated the latest work from
upstream).

-Peff

Re: [PATCH] push: disable lazy --force-with-lease by default

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 06 2017, Junio C. Hamano jotted:

> "git push --force-with-lease=:" makes sure that
> there is no unexpected changes to the branch at the remote while you
> prepare a rewrite based on the old state of the branch.  This
> feature came with an experimental option that allows : part
> to be omitted by using the tip of remote-tracking branch that
> corresponds to the .
>
> It turns out that some people use third-party tools that fetch from
> remote and update the remote-tracking branches behind users' back,
> defeating the safety relying on the stability of the remote-tracking
> branches.  We have some warning text that was meant to be scary
> sounding in our documentation, but nevertheless people seem to be
> bitten.  cf. 
> https://public-inbox.org/git/1491617750.2149.10.ca...@mattmccutchen.net/
> for a recent example.
>
> Let's disable the form that relies on the stability of remote-tracking
> branches by default, and allow users who _know_ their remote-tracking
> branches are stable to enable it with a configuration variable.
>
> This problem was predicted from the very beginning; see 28f5d176
> (remote.c: add command line option parser for "--force-with-lease",
> 2013-07-08).
>
> Signed-off-by: Junio C Hamano 
> ---
>
>  * This is a bit overdue safety fix that we should have done long
>time ago.  If we had this, I do not think it makes it riskier to
>forbid --force and tell people to use --force-with-lease.
>
>  Documentation/config.txt   |  5 +
>  Documentation/git-push.txt |  5 +++--
>  builtin/send-pack.c|  5 +
>  remote.c   | 16 
>  remote.h   |  2 ++
>  send-pack.c|  1 +
>  t/t5533-push-cas.sh| 19 +--
>  transport-helper.c |  5 +
>  transport.c|  5 +
>  9 files changed, 55 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 06898a7498..2f929315a2 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2588,6 +2588,11 @@ new default).
>
>  --
>
> +push.allowLazyForceWithLease::
> + If set to true, allow the `--force-with-lease` option
> + without the expected object name (i.e. expecting the objects
> + at the tip of corresponding remote-tracking branches).
> +

Just a note on the implementation. Re what I mentioned in
871spxchvm@gmail.com it would be more consistent to add a
--lazy-force-with-lease option, and have a corresponding
push.LazyForceWithLease config, which we'd turn off by default.

Then if/when I polish the patch to make CLI options configurable this
doesn't have to be handled by a special case, either by code or in the
mind of users.

But perhaps adding new CLI options is a bit too much of a hassle to
maintain such consistency.

Re: [PATCH] push: disable lazy --force-with-lease by default

[Stefan Haller]

Junio C Hamano  wrote:

> It turns out that some people use third-party tools that fetch from
> remote and update the remote-tracking branches behind users' back,
> defeating the safety relying on the stability of the remote-tracking
> branches.

Third-party tools are not the only problem. They may make the problem
more likely to occur, but it can also happen without them. (See below.)

> Let's disable the form that relies on the stability of remote-tracking
> branches by default, and allow users who _know_ their remote-tracking
> branches are stable to enable it with a configuration variable.

I'm wondering if people who claim they know they are safe really do.
Elsewhere in the other thread somebody said "I only ever explicitly
fetch, so I know I'm safe". Are you sure?

Consider this example:

$ git checkout the-branch-i-am-collaborating-on-with-my-collegue
$ git pull # make sure I have their latest work
$ git rebase -i ... # do some history rewriting
# OK, so as we need to force-push anyway, let's take the opportunity and
# rebase onto the latest master:
$ git fetch # get latest master
$ git rebase origin/master
$ git push --force-with-lease

This is a very common thing to do at my workplace. And it's unsafe,
because the git fetch may move the remote-tracking branch of the branch
I'm working on.

To make this safe, I guess you'd have to replace "git fetch" with
something like
$ git fetch refs/heads/master:refs/remotes/origin/master

Personally I have never used this form of fetch myself, and I'd be
surprised if any of my coworkers even know it exists.

So know you could decide that _any_ fetch is unsafe, and never use it;
only use git pull. You are still not safe:

$ git checkout the-branch-i-am-collaborating-on-with-my-collegue
$ git pull
$ git rebase -i
# Now another collegue walks in and asks me to look at the regression
# they just introduced on some other branch, so I do
$ git checkout that-other-branch
$ git pull
$ 
$ 
# go back to what I was doing:
$ git checkout the-branch-i-am-collaborating-on-with-my-collegue
$ git push --force-with-lease

Again, the git pull may have moved the remote-tracking branch of the
branch that I want to force-push. Again, it could be solved by given an
explicit refspec to git pull, but few people ever do this in my
experience, and I certainly never want to.

What I'm getting at is that there's a lot of things that you have to
remember to not do in order to make --force-with-lease without parameter
a useful tool.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

Re: [PATCH v2 6/7] reflog-walk: stop using fake parents

[Jeff King]

On Fri, Jul 07, 2017 at 05:14:07AM -0400, Jeff King wrote:

> @@ -3132,7 +3132,10 @@ static struct commit *get_revision_1(struct rev_info 
> *revs)
>   if (revs->max_age != -1 &&
>   (commit->date < revs->max_age))
>   continue;
> - if (add_parents_to_list(revs, commit, >commits, 
> NULL) < 0) {
> +
> + if (revs->reflog_info)
> + try_to_simplify_commit(revs, commit);
> + else if (add_parents_to_list(revs, commit, 
> >commits, NULL) < 0) {
>   if (!revs->ignore_missing_links)
>   die("Failed to traverse parents of 
> commit %s",
>   
> oid_to_hex(>object.oid));

There's one other subtle change from v1 here. In my original, we called
try_to_simplify_commit() much later, and then we had to check whether it
marked the commit as TREESAME. That felt really hacky to me, and the
reason is because I was using the function wrong. The intent is for
try_to_simplify to be called earlier (i.e., here), when we start looking
at the commit's parents. And then the TREESAME flag it sets is later
picked up by get_commit_action(), which is called by simplify_commit(),
which is used later in get_revision_1().

So by calling try_to_simplify here, where it would normally be called
for a non-reflog walk, that's all we have to do. I don't think the
original patch had any visible bugs, but this way is much cleaner and
more future-proof.

-Peff

Re: [PATCH v2 1/7] t1414: document some reflog-walk oddities

[Jeff King]

On Fri, Jul 07, 2017 at 05:06:10AM -0400, Jeff King wrote:

> +test_expect_failure 'walking multiple reflogs shows all' '
> + # We expect to see all entries for all reflogs, but interleaved by
> + # date, with order no the command line breaking ties. We
> + # can use "sort" on the separate lists to generate this,
> + # but note two tricks:
> + #
> + #   1. We use "{" as the delimiter, which lets us skip to the reflog
> + #  date specifier as our second field, and then our "-n" numeric
> + #  sort ignores the bits after the timestamp.
> + #
> + #   2. POSIX leaves undefined whether this is a stable sort or not. So
> + #  we use "-k 1" to ensure that we see HEAD before master before
> + #  side when breaking ties.
> + {
> + do_walk --date=unix HEAD &&
> + do_walk --date=unix side &&
> + do_walk --date=unix master
> + } >expect.raw &&
> + sort -t "{" -k 2nr -k 1 expect &&

I won't lie; the contortions needed for this "sort" made me a little
sick to my stomach.

It would be much easier if we could do something like:

  git log -g --format="%gt %gd"

and just get the reflog timestamp separately. But we never added %gt, so
we'd have to munge it with sed or perl.

One thing that perl would buy is that we could rely on a stable sort,
and ask for an order besides alphabetical. I.e., we could ask for:

  do_walk side master HEAD

and confirm that "side" comes before "HEAD", even though it doesn't
byte-wise sort.

I didn't think writing a perl snippet was worth the trouble for that
minor benefit, though probably it would be shorter than the comment I
needed to explain the "sort" invocation. ;)

-Peff

[PATCH v2 7/7] reflog-walk: apply --since/--until to reflog dates

[Jeff King]

When doing a reflog walk, we use the commit's date to
do any date limiting. In earlier versions of Git, this could
lead to nonsense results, since a skipped commit would
truncate the traversal. So a sequence like:

  git commit ...
  git checkout week-old-branch
  git checkout -
  git log -g --since=1.day.ago

would stop at the week-old-branch, even though the "git
commit" entry further back is still interesting.

As of the prior commit, which uses a parent-less traversal
of the reflog, you get the whole reflog minus any commits
whose dates do not match the specified options. This is
arguably useful, as you could scan the reflogs for commits
that originated in a certain range.

But more likely a user doing a reflog walk wants to limit
based on the reflog entries themselves. You can simulate
--until with:

  git log -g @{1.day.ago}

but there's no way to ask Git to traverse only back to a
certain date. E.g.:

  # show me reflog entries from the past day
  git log -g --since=1.day.ago

This patch teaches the revision machinery to prefer the
reflog entry dates to the commit dates when doing a reflog
walk. Technically this is a change in behavior that affects
plumbing, but the previous behavior was so buggy that it's
unlikely anyone was relying on it.

Signed-off-by: Jeff King 
---
 reflog-walk.c  | 12 
 reflog-walk.h  |  1 +
 revision.c | 19 ---
 t/t1414-reflog-walk.sh | 26 ++
 4 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index fbee9e0126..74ebe5148f 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -264,6 +264,18 @@ const char *get_reflog_ident(struct reflog_walk_info 
*reflog_info)
return info->email;
 }
 
+timestamp_t get_reflog_timestamp(struct reflog_walk_info *reflog_info)
+{
+   struct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;
+   struct reflog_info *info;
+
+   if (!commit_reflog)
+   return 0;
+
+   info = _reflog->reflogs->items[commit_reflog->recno+1];
+   return info->timestamp;
+}
+
 void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,
 const struct date_mode *dmode, int force_date)
 {
diff --git a/reflog-walk.h b/reflog-walk.h
index 373388cd14..7553c448fe 100644
--- a/reflog-walk.h
+++ b/reflog-walk.h
@@ -13,6 +13,7 @@ extern void show_reflog_message(struct reflog_walk_info 
*info, int,
 extern void get_reflog_message(struct strbuf *sb,
struct reflog_walk_info *reflog_info);
 extern const char *get_reflog_ident(struct reflog_walk_info *reflog_info);
+extern timestamp_t get_reflog_timestamp(struct reflog_walk_info *reflog_info);
 extern void get_reflog_selector(struct strbuf *sb,
struct reflog_walk_info *reflog_info,
const struct date_mode *dmode, int force_date,
diff --git a/revision.c b/revision.c
index 587199739a..41b4375c3c 100644
--- a/revision.c
+++ b/revision.c
@@ -2965,6 +2965,18 @@ static inline int want_ancestry(const struct rev_info 
*revs)
return (revs->rewrite_parents || revs->children.name);
 }
 
+/*
+ * Return a timestamp to be used for --since/--until comparisons for this
+ * commit, based on the revision options.
+ */
+static timestamp_t comparison_date(const struct rev_info *revs,
+  struct commit *commit)
+{
+   return revs->reflog_info ?
+   get_reflog_timestamp(revs->reflog_info) :
+   commit->date;
+}
+
 enum commit_action get_commit_action(struct rev_info *revs, struct commit 
*commit)
 {
if (commit->object.flags & SHOWN)
@@ -2975,8 +2987,9 @@ enum commit_action get_commit_action(struct rev_info 
*revs, struct commit *commi
return commit_show;
if (commit->object.flags & UNINTERESTING)
return commit_ignore;
-   if (revs->min_age != -1 && (commit->date > revs->min_age))
-   return commit_ignore;
+   if (revs->min_age != -1 &&
+   comparison_date(revs, commit) > revs->min_age)
+   return commit_ignore;
if (revs->min_parents || (revs->max_parents >= 0)) {
int n = commit_list_count(commit->parents);
if ((n < revs->min_parents) ||
@@ -3130,7 +3143,7 @@ static struct commit *get_revision_1(struct rev_info 
*revs)
 */
if (!revs->limited) {
if (revs->max_age != -1 &&
-   (commit->date < revs->max_age))
+   comparison_date(revs, commit) < revs->max_age)
continue;
 
if (revs->reflog_info)
diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh
index c4c53bd209..360754959e 100755
--- a/t/t1414-reflog-walk.sh
+++ b/t/t1414-reflog-walk.sh
@@ -91,6 +91,32 @@ test_expect_success 'date-limiting does not interfere with 
other logs' '

[PATCH v2 6/7] reflog-walk: stop using fake parents

[Jeff King]

The reflog-walk system works by putting a ref's tip into the
pending queue, and then "traversing" the reflog by
pretending that the parent of each commit is the previous
reflog entry.

This causes a number of user-visible oddities, as documented
in t1414 (and the commit message which introduced it). We
can fix all of them in one go by replacing the fake-reflog
system with a much simpler one: just keeping a list of
reflogs to show, and walking through them entry by entry.

The implementation is fairly straight-forward, but there are
a few items to note:

  1. We obviously must skip calling add_parents_to_list()
 when we are traversing reflogs, since we do not want to
 walk the original parents at all.  As a result, we must call
 try_to_simplify_commit() ourselves.

 There are other parts of add_parents_to_list() we skip,
 as well, but none of them should matter for a reflog
 traversal:

   -  We do not allow UNINTERESTING commits, nor
  symmetric ranges (and we bail when these are used
  with "-g").

   - Using --source makes no sense, since we aren't
 traversing. The reflog selector shows the same
 information with more detail.

   - Using --first-parent is still sensible, since you
 may want to see the first-parent diff for each
 entry. But since we're not traversing, we don't
 need to cull the parent list here.

  2. Since we now just walk the reflog entries themselves,
 rather than starting with the ref tip, we now look at
 the "new" field of each entry rather than the "old"
 (i.e., we are showing entries, not faking parents).
 This removes all of the tricky logic around skipping
 past root commits.

 But note that we have no way to show an entry with the
 null sha1 in its "new" field (because such a commit
 obviously does not exist). Normally this would not
 happen, since we delete reflogs along with refs, but
 there is one special case. When we rename the currently
 checked out branch, we write two reflog entries into
 the HEAD log: one where the commit goes away, and
 another where it comes back.

 Prior to this commit, we show both entries with
 identical reflog messages. After this commit, we show
 only the "comes back" entry. See the update in t3200
 which demonstrates this.

 Arguably either is fine, as the whole double-entry
 thing is a bit hacky in the first place. And until a
 recent fix, we truncated the traversal in such a case
 anyway, which was _definitely_ wrong.

  3. We show individual reflogs in order, but choose which
 reflog to show at each stage based on which has the
 most recent timestamp.  This interleaves the output
 from multiple reflogs based on date order, which is
 probably what you'd want with limiting like "-n 30".

 Note that the implementation aims for simplicity. It
 does a linear walk over the reflog queue for each
 commit it pulls, which may perform badly if you
 interleave an enormous number of reflogs. That seems
 like an unlikely use case; if we did want to handle it,
 we could probably keep a priority queue of reflogs,
 ordered by the timestamp of their current tip entry.

Signed-off-by: Jeff King 
---
 reflog-walk.c  | 137 ++---
 reflog-walk.h  |   4 +-
 revision.c |  27 +-
 t/t1414-reflog-walk.sh |  12 ++---
 t/t3200-branch.sh  |   3 +-
 5 files changed, 75 insertions(+), 108 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index 98c2f42de9..fbee9e0126 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -94,45 +94,6 @@ static int get_reflog_recno_by_time(struct complete_reflogs 
*array,
return -1;
 }
 
-struct commit_info_lifo {
-   struct commit_info {
-   struct commit *commit;
-   void *util;
-   } *items;
-   int nr, alloc;
-};
-
-static struct commit_info *get_commit_info(struct commit *commit,
-   struct commit_info_lifo *lifo, int pop)
-{
-   int i;
-   for (i = 0; i < lifo->nr; i++)
-   if (lifo->items[i].commit == commit) {
-   struct commit_info *result = >items[i];
-   if (pop) {
-   if (i + 1 < lifo->nr)
-   memmove(lifo->items + i,
-   lifo->items + i + 1,
-   (lifo->nr - i) *
-   sizeof(struct commit_info));
-   lifo->nr--;
-   }
-   return result;
-   }
-   return NULL;
-}
-
-static void add_commit_info(struct commit *commit, void *util,
-   struct commit_info_lifo *lifo)
-{
-   struct commit_info *info;
-   

[PATCH v2 4/7] get_revision_1(): replace do-while with an early return

[Jeff King]

The get_revision_1() function tries to avoid entering its
main loop at all when there are no commits to look at. But
it's perfectly safe to call pop_commit() on an empty list
(in which case it will return NULL). Switching to an early
return from the loop lets us skip repeating the loop
condition before we enter the do-while. That will get more
important when we start pulling reflog-walk commits from a
source besides the revs->commits queue, as that condition
will get much more complicated.

Signed-off-by: Jeff King 
---
 revision.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/revision.c b/revision.c
index 6678de04d9..4019e8cf23 100644
--- a/revision.c
+++ b/revision.c
@@ -3111,12 +3111,12 @@ static void track_linear(struct rev_info *revs, struct 
commit *commit)
 
 static struct commit *get_revision_1(struct rev_info *revs)
 {
-   if (!revs->commits)
-   return NULL;
-
-   do {
+   while (1) {
struct commit *commit = pop_commit(>commits);
 
+   if (!commit)
+   return NULL;
+
if (revs->reflog_info) {
save_parents(revs, commit);
fake_reflog_parent(revs->reflog_info, commit);
@@ -3150,8 +3150,7 @@ static struct commit *get_revision_1(struct rev_info 
*revs)
track_linear(revs, commit);
return commit;
}
-   } while (revs->commits);
-   return NULL;
+   }
 }
 
 /*
-- 
2.13.2.1000.g8590c1af5d

[PATCH v2 5/7] rev-list: check reflog_info before showing usage

[Jeff King]

When git-rev-list sees no pending commits, it shows a usage
message. This works even when reflog-walking is requested,
because the reflog-walk code currently puts the reflog tips
into the pending queue.

In preparation for refactoring the reflog-walk code, let's
explicitly check whether we have any reflogs to walk. For
now this is a noop, but the existing reflog tests will make
sure that it kicks in after the refactoring. Likewise, we'll
add a test that "rev-list -g" without specifying any reflogs
continues to fail (so that we know our check does not kick
in too aggressively).

Note that the implementation needs to go into its own
sub-function, as the walk code does not expose its innards
outside of reflog-walk.c.

Signed-off-by: Jeff King 
---
 builtin/rev-list.c | 3 ++-
 reflog-walk.c  | 5 +
 reflog-walk.h  | 2 ++
 t/t1414-reflog-walk.sh | 4 
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 95d84d5cda..53a746dd89 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -11,6 +11,7 @@
 #include "graph.h"
 #include "bisect.h"
 #include "progress.h"
+#include "reflog-walk.h"
 
 static const char rev_list_usage[] =
 "git rev-list [OPTION] ... [ -- paths... ]\n"
@@ -348,7 +349,7 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
/* Only --header was specified */
revs.commit_format = CMIT_FMT_RAW;
 
-   if ((!revs.commits &&
+   if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
 (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
  !revs.pending.nr)) ||
revs.diff)
diff --git a/reflog-walk.c b/reflog-walk.c
index 081f89b70d..98c2f42de9 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -365,3 +365,8 @@ void show_reflog_message(struct reflog_walk_info 
*reflog_info, int oneline,
strbuf_release();
}
 }
+
+int reflog_walk_empty(struct reflog_walk_info *info)
+{
+   return !info || !info->reflogs.nr;
+}
diff --git a/reflog-walk.h b/reflog-walk.h
index 27886f793e..af32361072 100644
--- a/reflog-walk.h
+++ b/reflog-walk.h
@@ -20,4 +20,6 @@ extern void get_reflog_selector(struct strbuf *sb,
const struct date_mode *dmode, int force_date,
int shorten);
 
+extern int reflog_walk_empty(struct reflog_walk_info *walk);
+
 #endif
diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh
index 6426829352..289c502725 100755
--- a/t/t1414-reflog-walk.sh
+++ b/t/t1414-reflog-walk.sh
@@ -102,4 +102,8 @@ test_expect_failure 'walk prefers reflog to ref tip' '
test_cmp expect actual
 '
 
+test_expect_success 'rev-list -g complains when there are no reflogs' '
+   test_must_fail git rev-list -g
+'
+
 test_done
-- 
2.13.2.1000.g8590c1af5d

[PATCH v2 3/7] log: do not free parents when walking reflog

[Jeff King]

When we're doing a reflog walk (instead of walking the
actual parent pointers), we may see commits multiple times.
For this reason, we hold on to the commit buffer for each
commit rather than freeing it after we've showed the commit.

We should do the same for the parent list. Right now this is
just a minor optimization. But once we refactor how reflog
walks are performed, keeping the parents will avoid
confusing us the second time we see the commit.

Signed-off-by: Jeff King 
---
 builtin/log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 8ca1de9894..9c8bb3b5c3 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -374,9 +374,9 @@ static int cmd_log_walk(struct rev_info *rev)
if (!rev->reflog_info) {
/* we allow cycles in reflog ancestry */
free_commit_buffer(commit);
+   free_commit_list(commit->parents);
+   commit->parents = NULL;
}
-   free_commit_list(commit->parents);
-   commit->parents = NULL;
if (saved_nrl < rev->diffopt.needed_rename_limit)
saved_nrl = rev->diffopt.needed_rename_limit;
if (rev->diffopt.degraded_cc_to_c)
-- 
2.13.2.1000.g8590c1af5d

[PATCH v2 2/7] revision: disallow reflog walking with revs->limited

[Jeff King]

The reflog-walk code doesn't work with limit_list().  That
function traverses down the real history graph, not the fake
reflog history that get_revision() returns. So it's not
going to actually examine all of the commits we're going to
show, because we'd add them to the pending list only during
the actual traversal.

In practice this limitation doesn't really matter, because
the options that require list-limiting generally need
UNINTERESTING endpoints or symmetric ranges, which already
are forbidden for reflog walks. Still, there are likely some
corner cases that would behave oddly. We're better off to
warn the user that we can't fulfill their request than to
generate potentially wrong output.

This will also make it easier to refactor the reflog-walking
code, because it eliminates a whole area of corner cases
we'd have to consider (that already don't work anyway).

Signed-off-by: Jeff King 
---
 revision.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/revision.c b/revision.c
index e181ad1b70..6678de04d9 100644
--- a/revision.c
+++ b/revision.c
@@ -2366,6 +2366,8 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
 
if (revs->reverse && revs->reflog_info)
die("cannot combine --reverse with --walk-reflogs");
+   if (revs->reflog_info && revs->limited)
+   die("cannot combine --walk-reflogs with history-limiting 
options");
if (revs->rewrite_parents && revs->children.name)
die("cannot combine --parents and --children");
 
-- 
2.13.2.1000.g8590c1af5d

[PATCH v2 1/7] t1414: document some reflog-walk oddities

[Jeff King]

Since its inception, the general strategy of the reflog-walk
code has been to start with the tip commit for the ref, and
as we traverse replace each commit's parent pointers with
fake parents pointing to the previous reflog entry.

This lets us traverse the reflog as if it were a real
history, but it has some user-visible oddities. Namely:

  1. The fake parents are used for commit selection and
 display. So for example, "--merges" or "--no-merges"
 are not useful, because the history appears as a linear
 string of commits. Likewise, pathspec limiting is based
 on the diff between adjacent entries, not the changes
 actually introduced by a commit.

 These are often the same (e.g., because the entry was
 just running "git commit" and the adjacent entry _is_
 the true parent), but it may not be in several common
 cases. For instance, using "git reset" to jump around
 history, or "git checkout" to move HEAD.

  2. We reverse-map each commit back to its reflog. So when
 it comes time to show commit X, we say "a-ha, we added
 X because it was at the tip of the 'foo' reflog, so
 let's show the foo reflog". But this leads to nonsense
 results when you ask to traverse multiple reflogs: if
 two reflogs have the same tip commit, we only map back
 to one of them.  Instead, we should show both.

  3. If the tip of the reflog and the ref tip disagree on
 the current value, we show the ref tip but give no
 indication of the value in the reflog.  This situation
 isn't supposed to happen (since any ref update should
 touch the reflog). But if it does, given that the
 requested operation is to show the reflog, it makes
 sense to prefer that.

This commit adds a new script with several expect_failure
tests to demonstrate the problems.  This could be part of
the existing t1411, but it's a bit easier to start from a
fresh state, where we know exactly what will be in the log.

Since the new multiple-reflog tests are checking the actual
output, we can drop the "make sure we don't segfault" tests
from t1411, which are a strict subset of what we're doing
here.

Signed-off-by: Jeff King 
---
 t/t1411-reflog-show.sh |  10 -
 t/t1414-reflog-walk.sh | 105 +
 2 files changed, 105 insertions(+), 10 deletions(-)
 create mode 100755 t/t1414-reflog-walk.sh

diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index b9cb76654b..6ac7734d79 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh
@@ -171,14 +171,4 @@ test_expect_success 'reflog exists works' '
! git reflog exists refs/heads/nonexistent
 '
 
-# The behavior with two reflogs is buggy and the output is in flux; for now
-# we're just checking that the program works at all without segfaulting.
-test_expect_success 'showing multiple reflogs works' '
-   git log -g HEAD HEAD >actual
-'
-
-test_expect_success 'showing multiple reflogs with an old date' '
-   git log -g HEAD@{1979-01-01} HEAD >actual
-'
-
 test_done
diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh
new file mode 100755
index 00..6426829352
--- /dev/null
+++ b/t/t1414-reflog-walk.sh
@@ -0,0 +1,105 @@
+#!/bin/sh
+
+test_description='various tests of reflog walk (log -g) behavior'
+. ./test-lib.sh
+
+test_expect_success 'set up some reflog entries' '
+   test_commit one &&
+   test_commit two &&
+   git checkout -b side HEAD^ &&
+   test_commit three &&
+   git merge --no-commit master &&
+   echo evil-merge-content >>one.t &&
+   test_tick &&
+   git commit --no-edit -a
+'
+
+do_walk () {
+   git log -g --format="%gd %gs" "$@"
+}
+
+sq="'"
+test_expect_success 'set up expected reflog' '
+   cat >expect.all <<-EOF
+   HEAD@{0} commit (merge): Merge branch ${sq}master${sq} into side
+   HEAD@{1} commit: three
+   HEAD@{2} checkout: moving from master to side
+   HEAD@{3} commit: two
+   HEAD@{4} commit (initial): one
+   EOF
+'
+
+test_expect_success 'reflog walk shows expected logs' '
+   do_walk >actual &&
+   test_cmp expect.all actual
+'
+
+test_expect_failure 'reflog can limit with --no-merges' '
+   grep -v merge expect.all >expect &&
+   do_walk --no-merges >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure 'reflog can limit with pathspecs' '
+   grep two expect.all >expect &&
+   do_walk -- two.t >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure 'pathspec limiting handles merges' '
+   # we pick up:
+   #   - the initial commit of one
+   #   - the checkout back to commit one
+   #   - the evil merge which touched one
+   sed -n "1p;3p;5p" expect.all >expect &&
+   do_walk -- one.t >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure '--parents shows true parents' '
+   # convert newlines to spaces
+   echo $(git rev-parse HEAD HEAD^1 HEAD^2) 

[PATCH v2 0/7] fixing reflog-walk oddities

[Jeff King]

On Fri, Jul 07, 2017 at 04:36:36AM -0400, Jeff King wrote:

> Here's an updated version of the bug-fix patch, along with the fix for
> the problem that Eric noticed, and some other problems I noticed while
> fixing that one. So I've split these immediate fixes for maint off into
> their own series.

And here's the parent-less walk for master. This must be applied on a
merge of the bug-fix series to the current master. I didn't do it all on
"maint" because a lot of it depends on the timestamp_t work which is
only in master. And trying to use "unsigned long" and merge that up to
master correctly is error-prone. It not only doesn't create textual
conflicts, but the semantic conflicts it generates are really subtle and
only break on certain systems.

This should address all of the comments on v1, including the
multi-reflog iteration order and the --since/--until bits. There are a
few new fixes, too.

  [v2 1/7]: t1414: document some reflog-walk oddities

The big change is that this expects the interleaved order in the
multi-reflog test. There are a few updated comments and some
adaptations to cover some bits from the bug-fix series.

  [v2 2/7]: revision: disallow reflog walking with revs->limited

This is new, and just cleanly disallows some already-broken cases.

  [v2 3/7]: log: do not free parents when walking reflog
  [v2 4/7]: get_revision_1(): replace do-while with an early return
  [v2 5/7]: rev-list: check reflog_info before showing usage

These ones are the same as before.

  [v2 6/7]: reflog-walk: stop using fake parents

The big change here is the interleaved output. This is largely what
I showed earlier, but with a few cleanups.

  [v2 7/7]: reflog-walk: apply --since/--until to reflog dates

This is new, and is a cleaned-up version of what I showed earlier.
See the commit message for some discussion. These options _do_
actually work sanely after 6/7, but I think the semantics given here
are probably more useful and what people would expect.

 builtin/log.c  |   4 +-
 builtin/rev-list.c |   3 +-
 reflog-walk.c  | 152 ++---
 reflog-walk.h  |   7 ++-
 revision.c |  57 ---
 t/t1411-reflog-show.sh |  10 
 t/t1414-reflog-walk.sh | 135 +++
 t/t3200-branch.sh  |   3 +-
 8 files changed, 249 insertions(+), 122 deletions(-)
 create mode 100755 t/t1414-reflog-walk.sh

-Peff

[PATCH v2 4/4] reflog-walk: include all fields when freeing complete_reflogs

[Jeff King]

When we encounter an error adding reflogs for a walk, we try
to free any logs we have read. But we didn't free all
fields, meaning that we could in theory leak all of the
"items" array (which would consitute the bulk of the
allocated memory).

This patch adds a helper which frees all of the entries and
uses it as appropriate.

As it turns out, the leak seems impossible to trigger with
the current code. Of the three error paths that free the
complete_reflogs struct, two only kick in when the items
array is empty, and the third was removed entirely in the
previous commit.

So this patch should be a noop in terms of behavior, but it
fixes a potential maintenance headache should anybody add a
new error path and copy the partial-free code. Which is
what happened in 5026b47175 (add_reflog_for_walk: avoid
memory leak, 2017-05-04), though its leaky call was the
third one that was recently removed.

Signed-off-by: Jeff King 
---
 reflog-walk.c | 26 ++
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index 2a43537326..ba72020fc3 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -38,6 +38,22 @@ static int read_one_reflog(struct object_id *ooid, struct 
object_id *noid,
return 0;
 }
 
+static void free_complete_reflog(struct complete_reflogs *array)
+{
+   int i;
+
+   if (!array)
+   return;
+
+   for (i = 0; i < array->nr; i++) {
+   free(array->items[i].email);
+   free(array->items[i].message);
+   }
+   free(array->items);
+   free(array->ref);
+   free(array);
+}
+
 static struct complete_reflogs *read_complete_reflog(const char *ref)
 {
struct complete_reflogs *reflogs =
@@ -189,20 +205,14 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
if (ret > 1)
free(b);
else if (ret == 1) {
-   if (reflogs) {
-   free(reflogs->ref);
-   free(reflogs);
-   }
+   free_complete_reflog(reflogs);
free(branch);
branch = b;
reflogs = read_complete_reflog(branch);
}
}
if (!reflogs || reflogs->nr == 0) {
-   if (reflogs) {
-   free(reflogs->ref);
-   free(reflogs);
-   }
+   free_complete_reflog(reflogs);
free(branch);
return -1;
}
-- 
2.13.2.1000.g8590c1af5d

[PATCH v2 3/4] reflog-walk: don't free reflogs added to cache

[Jeff King]

The add_reflog_for_walk() function keeps a cache mapping
refnames to their reflog contents. We use a cached reflog
entry if available, and otherwise allocate and store a new
one.

Since 5026b47175 (add_reflog_for_walk: avoid memory leak,
2017-05-04), when we hit an error parsing a date-based
reflog spec, we free the reflog memory but leave the cache
entry pointing to the now-freed memory.

We can fix this by just leaving the memory intact once it
has made it into the cache. This may leave an unused entry
in the cache, but that's OK. And it means we also catch a
similar situation: we may not have allocated at all in this
invocation, but simply be pointing to a cached entry from a
previous invocation (which is relying on that entry being
present).

The new test in t1411 exercises this case and fails when run
with --valgrind or ASan.

Signed-off-by: Jeff King 
---
 reflog-walk.c  | 4 
 t/t1411-reflog-show.sh | 4 
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index aec718beba..2a43537326 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -215,10 +215,6 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
if (recno < 0) {
commit_reflog->recno = get_reflog_recno_by_time(reflogs, 
timestamp);
if (commit_reflog->recno < 0) {
-   if (reflogs) {
-   free(reflogs->ref);
-   free(reflogs);
-   }
free(commit_reflog);
return -1;
}
diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index ae96eeb66a..b9cb76654b 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh
@@ -177,4 +177,8 @@ test_expect_success 'showing multiple reflogs works' '
git log -g HEAD HEAD >actual
 '
 
+test_expect_success 'showing multiple reflogs with an old date' '
+   git log -g HEAD@{1979-01-01} HEAD >actual
+'
+
 test_done
-- 
2.13.2.1000.g8590c1af5d

[PATCH v2 2/4] reflog-walk: duplicate strings in complete_reflogs list

[Jeff King]

As part of the add_reflog_to_walk() function, we keep a
string_list mapping refnames to their reflog contents. This
serves as a cache so that accessing the same reflog twice
requires only a single copy of the log in memory.

The string_list is initialized via xcalloc, meaning its
strdup_strings field is set to 0. But after inserting a
string into the list, we unconditionally call free() on the
string, leaving the list pointing to freed memory. If
another reflog is added (e.g., "git log -g HEAD HEAD"), then
the second one may have unpredictable results.

The extra free was added by 5026b47175 (add_reflog_for_walk:
avoid memory leak, 2017-05-04). Though if you look
carefully, you can see that the code was buggy even before
then. If we tried to read the reflogs by time but came up
with no entries, we exited with an error, freeing the string
in that code path. So the bug was harder to trigger, but
still there.

We can fix it by just asking the string list to make a copy
of the string. Technically we could fix the problem by not
calling free() on our string (and just handing over
ownership to the string list), but there are enough
conditionals that it's quite hard to figure out which code
paths need the free and which do not. Simpler is better
here.

The new test reliably shows the problem when run with
--valgrind or ASAN.

Signed-off-by: Jeff King 
---
 reflog-walk.c  | 1 +
 t/t1411-reflog-show.sh | 6 ++
 2 files changed, 7 insertions(+)

diff --git a/reflog-walk.c b/reflog-walk.c
index f7ffd1caa3..aec718beba 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -136,6 +136,7 @@ struct reflog_walk_info {
 void init_reflog_walk(struct reflog_walk_info **info)
 {
*info = xcalloc(1, sizeof(struct reflog_walk_info));
+   (*info)->complete_reflogs.strdup_strings = 1;
 }
 
 int add_reflog_for_walk(struct reflog_walk_info *info,
diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index 6ac7734d79..ae96eeb66a 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh
@@ -171,4 +171,10 @@ test_expect_success 'reflog exists works' '
! git reflog exists refs/heads/nonexistent
 '
 
+# The behavior with two reflogs is buggy and the output is in flux; for now
+# we're just checking that the program works at all without segfaulting.
+test_expect_success 'showing multiple reflogs works' '
+   git log -g HEAD HEAD >actual
+'
+
 test_done
-- 
2.13.2.1000.g8590c1af5d

[PATCH v2 1/4] reflog-walk: skip over double-null oid due to HEAD rename

[Jeff King]

Since 39ee4c6c2f (branch: record creation of renamed branch
in HEAD's log, 2017-02-20), a rename on the currently
checked out branch will create two entries in the HEAD
reflog: one where the branch goes away (switching to the
null oid), and one where it comes back (switching away from
the null oid).

This confuses the reflog-walk code. When walking backwards,
it first sees the null oid in the "old" field of the second
entry. Thanks to the "root commit" logic added by 71abeb753f
(reflog: continue walking the reflog past root commits,
2016-06-03), we keep looking for the next entry by scanning
the "new" field from the previous entry. But that field is
also null! We need to go just a tiny bit further, and look
at its "old" field. But with the current code, we decide the
reflog has nothing else to show and just give up. To the
user this looks like the reflog was truncated by the rename
operation, when in fact those entries are still there.

This patch does the absolute minimal fix, which is to look
back that one extra level and keep traversing.

The resulting behavior may not be the _best_ thing to do in
the long run (for example, we show both reflog entries each
with the same commit id), but it's a simple way to fix the
problem without risking further regressions.

Signed-off-by: Jeff King 
---
 reflog-walk.c |  2 ++
 t/t3200-branch.sh | 11 +++
 2 files changed, 13 insertions(+)

diff --git a/reflog-walk.c b/reflog-walk.c
index c63eb1a3fd..f7ffd1caa3 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, 
struct commit *commit)
/* a root commit, but there are still more entries to show */
reflog = _reflog->reflogs->items[commit_reflog->recno];
logobj = parse_object(reflog->noid.hash);
+   if (!logobj)
+   logobj = parse_object(reflog->ooid.hash);
}
 
if (!logobj || logobj->type != OBJ_COMMIT) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 48d152b9a9..dd37ac47c5 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -162,6 +162,17 @@ test_expect_success 'git branch -M baz bam should add 
entries to .git/logs/HEAD'
grep "^0\{40\}.*$msg$" .git/logs/HEAD
 '
 
+test_expect_success 'resulting reflog can be shown by log -g' '
+   oid=$(git rev-parse HEAD) &&
+   cat >expect <<-EOF &&
+   HEAD@{0} $oid $msg
+   HEAD@{1} $oid $msg
+   HEAD@{2} $oid checkout: moving from foo to baz
+   EOF
+   git log -g --format="%gd %H %gs" -3 HEAD >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'git branch -M baz bam should succeed when baz is checked 
out as linked working tree' '
git checkout master &&
git worktree add -b baz bazdir &&
-- 
2.13.2.1000.g8590c1af5d

[PATCH v2 0/4] reflog-walk fixes for maint

[Jeff King]

On Wed, Jul 05, 2017 at 03:55:08AM -0400, Jeff King wrote:

> The first patch is my original small fix with an extra test. I think
> that would be appropriate for 'maint'. Its behavior still has some
> quirks, but it avoids the confusion that you experienced and has a low
> risk of breaking anything else.
> 
> The rest of it replaces the fake-parent thing with a more
> straight-forward iteration over the reflogs (i.e., a cleanup of the
> further patches I've been posting). After digging into it and especially
> after writing the new tests, I think I've convinced myself that this is
> the right way forward.

Here's an updated version of the bug-fix patch, along with the fix for
the problem that Eric noticed, and some other problems I noticed while
fixing that one. So I've split these immediate fixes for maint off into
their own series.

These are based on maint itself, rather than Kyle's original commit that
introduces the double-null reflog, since some of the bugs came later in
the v2.13 cycle (if we really wanted to, we could split it again into
two more series, but I don't think it's worth the trouble).

  [1/4]: reflog-walk: skip over double-null oid due to HEAD rename

This is the fix for the pseudo-truncation in v2.13, and is the same
as the previous round.

  [2/4]: reflog-walk: duplicate strings in complete_reflogs list

This fixes Eric's bug, and is the same as what I showed earlier.
It's a triggerable use-after-free, which is why I think it's
important to get it into maint.

  [3/4]: reflog-walk: don't free reflogs added to cache

This is another use-after-free, though it's slightly harder to
trigger.

  [4/4]: reflog-walk: include all fields when freeing complete_reflogs

This one is an optional cleanup, but worth doing, I think.


 reflog-walk.c  | 33 +
 t/t1411-reflog-show.sh | 10 ++
 t/t3200-branch.sh  | 11 +++
 3 files changed, 42 insertions(+), 12 deletions(-)

-Peff

Re: [PATCH 2/6] t1414: document some reflog-walk oddities

[Jeff King]

On Thu, Jul 06, 2017 at 11:00:13PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I suspect that "--since 3.days" is still quite buggy (even with a single
> > reflog) because it checks commit timestamps and stops traversing when we
> > go too bar back. But in a reflog, the commits may be totally out of
> > order. I'm not sure what it should do. Either:
> >
> >   1. During a reflog walk, --since and --until should respect reflog
> >  timestamps instead of commit timestamps. You can already do
> >  "--until" there by simply starting the traversal later, but there's
> >  no way to cut it off with --since.
> >
> >   2. Limit commits shown by --since/--until as usual, but skip the "stop
> >  traversing" optimization when we see too many "old" commits. I.e.,
> >  omit a 4.days.ago commit, but keep walking to find other recent
> >  commits.
> 
> I think 1. is more logical, and I was imagining that it should be
> doable, now we are not constrained by (ab)using the commit_list with
> the fake-parent thing, but are pulling the entries directly from the
> reflog iterator and the timestamp would be already available to the
> iterator.
> 
> But I recall that the max_age and min_age cutoff is done long after
> a commit is pulled out of the "iterator mechanism" (be it the
> commit_list or your direct reflog iterator) by comparing
> commit->date with the cut-off, so it may be a bit more involved to
> arrange than I imagined X-<.  Hmph...

It's probably not too bad.

We do some of the limiting in limit_list(), which tries to mark commits
as UNINTERESTING. But I think in general that limit_list is incompatible
with reflog traversals (though I wouldn't be surprised if we fail to
flag all the options that set revs->limited as incompatible).

We handle max_age in get_revision_1() itself, which should be pretty
straightforward. For min_age, we do it in get_commit_action(), which
would need access to the reflog timestamp. But we do have the rev_info
there.

So something like the patch below would work, I think.

diff --git a/reflog-walk.c b/reflog-walk.c
index fbee9e0126..74ebe5148f 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -264,6 +264,18 @@ const char *get_reflog_ident(struct reflog_walk_info 
*reflog_info)
return info->email;
 }
 
+timestamp_t get_reflog_timestamp(struct reflog_walk_info *reflog_info)
+{
+   struct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;
+   struct reflog_info *info;
+
+   if (!commit_reflog)
+   return 0;
+
+   info = _reflog->reflogs->items[commit_reflog->recno+1];
+   return info->timestamp;
+}
+
 void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,
 const struct date_mode *dmode, int force_date)
 {
diff --git a/reflog-walk.h b/reflog-walk.h
index 373388cd14..7553c448fe 100644
--- a/reflog-walk.h
+++ b/reflog-walk.h
@@ -13,6 +13,7 @@ extern void show_reflog_message(struct reflog_walk_info 
*info, int,
 extern void get_reflog_message(struct strbuf *sb,
struct reflog_walk_info *reflog_info);
 extern const char *get_reflog_ident(struct reflog_walk_info *reflog_info);
+extern timestamp_t get_reflog_timestamp(struct reflog_walk_info *reflog_info);
 extern void get_reflog_selector(struct strbuf *sb,
struct reflog_walk_info *reflog_info,
const struct date_mode *dmode, int force_date,
diff --git a/revision.c b/revision.c
index 5fc01f2d26..c248a16974 100644
--- a/revision.c
+++ b/revision.c
@@ -2973,8 +2973,13 @@ enum commit_action get_commit_action(struct rev_info 
*revs, struct commit *commi
return commit_show;
if (commit->object.flags & UNINTERESTING)
return commit_ignore;
-   if (revs->min_age != -1 && (commit->date > revs->min_age))
-   return commit_ignore;
+   if (revs->min_age != -1) {
+   timestamp_t date = revs->reflog_info ?
+  get_reflog_timestamp(revs->reflog_info) :
+  commit->date;
+   if (date > revs->min_age)
+   return commit_ignore;
+   }
if (revs->min_parents || (revs->max_parents >= 0)) {
int n = commit_list_count(commit->parents);
if ((n < revs->min_parents) ||
@@ -3127,9 +3132,13 @@ static struct commit *get_revision_1(struct rev_info 
*revs)
 * that we'd otherwise have done in limit_list().
 */
if (!revs->limited) {
-   if (revs->max_age != -1 &&
-   (commit->date < revs->max_age))
-   continue;
+   if (revs->max_age != -1) {
+   timestamp_t date = revs->reflog_info ?
+  
get_reflog_timestamp(revs->reflog_info) :
+  

Re: [PATCH] push: add config option to --force-with-lease by default.

[Francesco Mazzoli]

On 6 July 2017 at 21:13, Junio C Hamano  wrote:
> By that logic, a hypothetical update to `--force` that makes 1/3 of
> the attempted forced push randomly would make it safer than the
> current `--force`, wouldn't it?

It would. However, this additional safety is not really meaningful to any
workflow, while the one added by `--force-with-lease` is.

> When third-party tools fetch and update remote-tracking branches
> behind the users' back, the safety based on the stability of
> remote-tracking branches are defeated.  And the biggest problem
> is that the way `--force-with-lease` misbehaves---it is not like
> it randomly and mistakenly stops the push that could go through;
> it lets through what shouldn't.
>
> See the other patch I sent just now---with something like that patch
> that lets those like you, who know their remote-tracking branches
> are reliable, use the lazy form, while disabling it by default for
> others (until they examine their situation and perhaps disable the
> problematic auto-fetching) in place, I do not think it is a bad idea
> to advertise --force-with-lease a safer option than --force (because
> those for whom it is not safer will not be able to use it).

Fair enough, I'm OK with enabling it with some config. I'd still like a
way to enable it by default if I want though.

Thanks,
Francesco

Re: [PATCH 2/6] t1414: document some reflog-walk oddities

[Junio C Hamano]

Jeff King  writes:

> I suspect that "--since 3.days" is still quite buggy (even with a single
> reflog) because it checks commit timestamps and stops traversing when we
> go too bar back. But in a reflog, the commits may be totally out of
> order. I'm not sure what it should do. Either:
>
>   1. During a reflog walk, --since and --until should respect reflog
>  timestamps instead of commit timestamps. You can already do
>  "--until" there by simply starting the traversal later, but there's
>  no way to cut it off with --since.
>
>   2. Limit commits shown by --since/--until as usual, but skip the "stop
>  traversing" optimization when we see too many "old" commits. I.e.,
>  omit a 4.days.ago commit, but keep walking to find other recent
>  commits.

I think 1. is more logical, and I was imagining that it should be
doable, now we are not constrained by (ab)using the commit_list with
the fake-parent thing, but are pulling the entries directly from the
reflog iterator and the timestamp would be already available to the
iterator.

But I recall that the max_age and min_age cutoff is done long after
a commit is pulled out of the "iterator mechanism" (be it the
commit_list or your direct reflog iterator) by comparing
commit->date with the cut-off, so it may be a bit more involved to
arrange than I imagined X-<.  Hmph...