Re: [PATCH v3] Makefile: replace perl/Makefile.PL with simple make rules

[Michael J Gruber]

Ævar Arnfjörð Bjarmason venit, vidit, dixit 12.12.2017 23:26:
> 
> On Tue, Dec 12 2017, Randall S. Becker jotted:
> 
>> -Original Message-
>> On December 10, 2017 4:14 PM, Ævar Arnfjörð Bjarmason wrote:
>> Subject: [PATCH v3] Makefile: replace perl/Makefile.PL with simple make rules
>>
>>> Replace the perl/Makefile.PL and the fallback perl/Makefile used under 
>>> NO_PERL_MAKEMAKER=NoThanks with a much simpler implementation heavily 
>>> inspired by how the i18n infrastructure's build process works[1].
>>> The reason for having the Makefile.PL in the first place is that it was 
>>> initially[2] building a perl C binding to interface with libgit, this 
>>> functionality, that was removed[3] before Git.pm ever made it to the master 
>>> branch.
>> 
>>
>> I would like to request that the we be careful that the git builds do not 
>> introduce arbitrary dependencies to CPAN. Some platforms (I can think of one 
>> off the top, being NonStop) does not provide for arbitrary additions to the 
>> supplied perl implementation as of yet. The assumption about being able to 
>> add CPAN modules may apply on some platforms but is not a general 
>> capability. I am humbly requesting that caution be used when adding 
>> dependencies. Being non-$DAYJOB responsible for the git port for NonStop, 
>> this scares me a bit, but I and my group can help validate the available 
>> modules used for builds.
>>
>> Note: we do not yet have CPAN's SCM so can't and don't use perl for access 
>> to git anyway - much that I've tried to change that.
>>
>> Please keep build dependencies to a minimum.
>>
>> Thanks for my and my whole team.
> 
> I think you should be happy with this patch then, and it doesn't add any
> more CPAN dependency than before, and sets up a framework (as discussed
> in [1]) where we can use more CPAN modules while not requiring packagers
> such as yourself to package CPAN modules.
> 
> However, it doesn't sound believable to me that even on NonStop you
> can't install any CPAN modules whatsoever.
> 
> That would also mean that this patch doesn't work for you, because it
> means that you either don't have anything resembling a hierarchical
> filesystem on which git can be installed in the first place (in which
> case it wouldn't work), or perl doesn't have an @INC to search through
> perl libs on on NonStop. What does:
> 
> perl -V
> 
> Return for you on that system?
> 
> If this patch works, and if at the bottom of `perl -V` you see some
> directories which you could write a package to drop some static *.pm
> files, then you can grab a *.tar.gz from CPAN such as the one for
> Error.pm[2] and arrange for the *.pm files contained within its lib/
> directory to be dropped into one of those @INC directories.
> 
> It may be that some aspect of the CPAN toolchain is broken for you, or
> even ExtUtils::MakeMaker, but you typically don't need that to package
> non-XS perl modules, certainly not any of the ones we've discussed
> possibly bundling up in git.git on-list recently. As a (very occasional)
> contributor to perl.git I'd be interested to know if that's what you
> mean is broken, and if so see if it could be fixed for you.
> 
> 1. 

Re: [PATCH 2/3] merge-base: return fork-point outside reflog

[Michael J Gruber]

Ekelhart Jakob venit, vidit, dixit 08.11.2017 09:52:
> Thank you for all the effort to fix this issue. Unfortunately, we are still 
> suffering from this and our workaround just stopped being sufficient.
> 
> We were wondering if there is any way to tell when this fix will be released?
> 
> BR Jakob

Soon (TM) :)

Term start kept me busy, but I'll try and resume dangling topics this
week or next.

It seems the consensus was that current functionality is as designed but
not necessarily as expected, and another mode "--fork-base" (that does
what I suggested as "fix") would meet these expectations. I would reuse
the documentation of the current mode as a description of the new mode
and add documentation for the existing mode ;)

Michael

Re: [PATCH 3/4] merge: --no-verify to bypass pre-merge hook

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 24.09.2017 01:48:
> Michael J Gruber <g...@grubix.eu> writes:
> 
>> From: Michael J Gruber <g...@drmicha.warpmail.net>
>>
>> Analogous to commit, introduce a '--no-verify' option which bypasses the
>> pre-merge hook. The shorthand '-n' is taken by the (non-existing)
>> '--no-stat' already.
>>
>> Signed-off-by: Michael J Gruber <g...@grubix.eu>
>> ---
> 
> It appears that some of the pieces in this patch, namely,
> D/git-merge.txt and D/merge-options.txt, belong to 2/4, where we are
> fixing up an earlier addition of --[no-]verify option to the
> command, to be updated to only add mention of pre-merge hook in this
> step?

Oh, sorry, rebaser error. I should also rewrite all commits to my
current e-mail.

> The end result looks good regardless, I would think, though.

Pending restructuring and attending to the other comments...

[PATCH 4/4] t7503: add tests for pre-merge-hook

[Michael J Gruber]

From: Michael J Gruber <g...@drmicha.warpmail.net>

Add tests which make sure that the pre-merge-hook is called when
present, allows/disallows merge commits depending on its return value
and is suppressed by "--no-verify".

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 t/t7503-pre-commit-hook.sh | 66 +-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b39d..36ae87f7ef 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -1,9 +1,22 @@
 #!/bin/sh
 
-test_description='pre-commit hook'
+test_description='pre-commit and pre-merge hooks'
 
 . ./test-lib.sh
 
+test_expect_success 'root commit' '
+
+   echo "root" > file &&
+   git add file &&
+   git commit -m "zeroth" &&
+   git checkout -b side &&
+   echo "foo" > foo &&
+   git add foo &&
+   git commit -m "make it non-ff" &&
+   git checkout master
+
+'
+
 test_expect_success 'with no hook' '
 
echo "foo" > file &&
@@ -12,6 +25,14 @@ test_expect_success 'with no hook' '
 
 '
 
+test_expect_success 'with no hook (merge)' '
+
+   git checkout side &&
+   git merge -m "merge master" master &&
+   git checkout master
+
+'
+
 test_expect_success '--no-verify with no hook' '
 
echo "bar" > file &&
@@ -20,15 +41,25 @@ test_expect_success '--no-verify with no hook' '
 
 '
 
+test_expect_success '--no-verify with no hook (merge)' '
+
+   git checkout side &&
+   git merge --no-verify -m "merge master" master &&
+   git checkout master
+
+'
+
 # now install hook that always succeeds
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
 HOOK="$HOOKDIR/pre-commit"
+MERGEHOOK="$HOOKDIR/pre-merge"
 mkdir -p "$HOOKDIR"
 cat > "$HOOK" <> file &&
@@ -46,11 +85,20 @@ test_expect_success '--no-verify with succeeding hook' '
 
 '
 
+test_expect_success '--no-verify with succeeding hook (merge)' '
+
+   git checkout side &&
+   git merge --no-verify -m "merge master" master &&
+   git checkout master
+
+'
+
 # now a hook that fails
 cat > "$HOOK" <

[PATCH 0/4] pre-merge hook

[Michael J Gruber]

Now that f8b863598c ("builtin/merge: honor commit-msg hook for merges",
2017-09-07) has landed, merge is getting closer to behaving like commit,
which is important because both are used to complete merges (automatic
vs. non-automatic).

This series revives an old suggestion of mine to make merge honor
pre-commit hook or a separate pre-merge hook. Since pre-commit does not
receive any arguments (which the hook could use tell between commit and
merge) and in order to keep existing behaviour (as opposed to the other
patch) this series introduces a pre-merge hook, with a sample hook that
simply calls the pre-commit hook.

commit and merge have very similar code paths. f8b863598c implemented
"--no-verify" for merge differently from the existing implementation in
commit. 2/4 changes that and could be first in this series, with the
remaining 3 squashed into 2 commits or 1. I've been rebasing and using
this series for years now, 2/4 is the new-comer which fixed the breakage
from f8b863598c that I encountered because of the no-verify
implementation. 

Michael J Gruber (4):
  git-merge: Honor pre-merge hook
  merge: do no-verify like commit
  merge: --no-verify to bypass pre-merge hook
  t7503: add tests for pre-merge-hook

 Documentation/git-merge.txt   |  2 +-
 Documentation/githooks.txt|  7 +
 Documentation/merge-options.txt   |  4 +++
 builtin/merge.c   | 17 --
 t/t7503-pre-commit-hook.sh| 66 ++-
 templates/hooks--pre-merge.sample | 13 
 6 files changed, 104 insertions(+), 5 deletions(-)
 create mode 100755 templates/hooks--pre-merge.sample

-- 
2.14.1.909.g0fa57a0913

[PATCH 3/4] merge: --no-verify to bypass pre-merge hook

[Michael J Gruber]

From: Michael J Gruber <g...@drmicha.warpmail.net>

Analogous to commit, introduce a '--no-verify' option which bypasses the
pre-merge hook. The shorthand '-n' is taken by the (non-existing)
'--no-stat' already.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 Documentation/git-merge.txt | 2 +-
 Documentation/githooks.txt  | 2 +-
 Documentation/merge-options.txt | 4 
 builtin/merge.c | 4 ++--
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 4df6431c34..5f0e1768e1 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 
 [verse]
 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
-   [-s ] [-X ] [-S[]]
+   [--no-verify] [-s ] [-X ] [-S[]]
[--[no-]allow-unrelated-histories]
[--[no-]rerere-autoupdate] [-m ] [...]
 'git merge' --abort
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 85bedd208c..969441d7a2 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -104,7 +104,7 @@ pre-merge
 
 This hook is invoked by 'git merge' when doing an automatic merge
 commit; it is equivalent to 'pre-commit' for a non-automatic commit
-for a merge.
+for a merge, and can be bypassed with the `--no-verify` option. 
 
 prepare-commit-msg
 ~~
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 4e32304301..75019d5919 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -74,6 +74,10 @@ merge.
 With --no-squash perform the merge and commit the result. This
 option can be used to override --squash.
 
+--no-verify::
+   This option bypasses the pre-merge and commit-msg hooks.
+   See also linkgit:githooks[5].
+
 -s ::
 --strategy=::
Use the given merge strategy; can be supplied more than
diff --git a/builtin/merge.c b/builtin/merge.c
index 7ba094ee87..c63510c199 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -237,7 +237,7 @@ static struct option builtin_merge_options[] = {
  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
files (default)")),
OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
-   OPT_BOOL(0, "no-verify", _verify, N_("bypass commit-msg hook")),
+   OPT_BOOL(0, "no-verify", _verify, N_("bypass pre-merge and 
commit-msg hooks")),
OPT_END()
 };
 
@@ -771,7 +771,7 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
struct strbuf msg = STRBUF_INIT;
const char *index_file = get_index_file();
 
-   if (run_commit_hook(0 < option_edit, index_file, "pre-merge", NULL))
+   if (!no_verify && run_commit_hook(0 < option_edit, index_file, 
"pre-merge", NULL))
abort_commit(remoteheads, NULL);
/*
 * Re-read the index as pre-merge hook could have updated it,
-- 
2.14.1.909.g0fa57a0913

[PATCH 1/4] git-merge: Honor pre-merge hook

[Michael J Gruber]

From: Michael J Gruber <g...@drmicha.warpmail.net>

git-merge does not honor the pre-commit hook when doing automatic merge
commits, and for compatibility reasons this is going to stay.

Introduce a pre-merge hook which is called for an automatic merge commit
just like pre-commit is called for a non-automatic merge commit (or any
other commit).

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 Documentation/githooks.txt|  7 +++
 builtin/merge.c   | 11 +++
 templates/hooks--pre-merge.sample | 13 +
 3 files changed, 31 insertions(+)
 create mode 100755 templates/hooks--pre-merge.sample

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 1bb4f92d4d..85bedd208c 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -99,6 +99,13 @@ All the 'git commit' hooks are invoked with the environment
 variable `GIT_EDITOR=:` if the command will not bring up an editor
 to modify the commit message.
 
+pre-merge
+~
+
+This hook is invoked by 'git merge' when doing an automatic merge
+commit; it is equivalent to 'pre-commit' for a non-automatic commit
+for a merge.
+
 prepare-commit-msg
 ~~
 
diff --git a/builtin/merge.c b/builtin/merge.c
index ab5ffe85e8..de254d466b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -769,6 +769,17 @@ static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
struct strbuf msg = STRBUF_INIT;
+   const char *index_file = get_index_file();
+
+   if (run_commit_hook(0 < option_edit, index_file, "pre-merge", NULL))
+   abort_commit(remoteheads, NULL);
+   /*
+* Re-read the index as pre-merge hook could have updated it,
+* and write it out as a tree.  We must do this before we invoke
+* the editor and after we invoke run_status above.
+*/
+   discard_cache();
+   read_cache_from(index_file);
strbuf_addbuf(, _msg);
strbuf_addch(, '\n');
if (squash)
diff --git a/templates/hooks--pre-merge.sample 
b/templates/hooks--pre-merge.sample
new file mode 100755
index 00..a6313e6d5c
--- /dev/null
+++ b/templates/hooks--pre-merge.sample
@@ -0,0 +1,13 @@
+#!/bin/sh
+#
+# An example hook script to verify what is about to be committed.
+# Called by "git merge" with no arguments.  The hook should
+# exit with non-zero status after issuing an appropriate message if
+# it wants to stop the commit.
+#
+# To enable this hook, rename this file to "pre-merge".
+
+. git-sh-setup
+test -x "$GIT_DIR/hooks/pre-commit" &&
+exec "$GIT_DIR/hooks/pre-commit"
+:
-- 
2.14.1.909.g0fa57a0913

[PATCH 2/4] merge: do no-verify like commit

[Michael J Gruber]

f8b863598c ("builtin/merge: honor commit-msg hook for merges", 2017-09-07)
introduced the no-verify to merge for bypassing the commit-msg hook,
though in a different way from the implementation in commit.c.

Change the implementation in merge.c to be the same as in merge.c so
that both do the same in the same way.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/merge.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index de254d466b..7ba094ee87 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -73,7 +73,7 @@ static int show_progress = -1;
 static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
-static int verify_msg = 1;
+static int no_verify;
 
 static struct strategy all_strategy[] = {
{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -237,7 +237,7 @@ static struct option builtin_merge_options[] = {
  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
files (default)")),
OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
-   OPT_BOOL(0, "verify", _msg, N_("verify commit-msg hook")),
+   OPT_BOOL(0, "no-verify", _verify, N_("bypass commit-msg hook")),
OPT_END()
 };
 
@@ -798,7 +798,7 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
abort_commit(remoteheads, NULL);
}
 
-   if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+   if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
  "commit-msg",
  git_path_merge_msg(), NULL))
abort_commit(remoteheads, NULL);
-- 
2.14.1.909.g0fa57a0913

Re: [PATCH 2/3] merge-base: return fork-point outside reflog

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 22.09.2017 03:49:
> Michael J Gruber <g...@grubix.eu> writes:
> 
>> Also, I'm undecided about about your reflog argument above - if we leave
>> "--fork-point" to be the current behaviour including Jeff's fix then the
>> documentation would need an even bigger overhaul, because it's neither
>> "reflog also" (as claimed in the doc) nor "reflog only" (as in the
>> original implementation) but "historical tips as inferred from the
>> current value and the reflog".
> 
> Even though things like "reflog only", "reflog also", may be
> something implementors may care about, they are irrelevant
> implementation details to the intended audience.  "The bases that
> are not in reflogs are ignored" _does_ matter, as it affects the
> outcome, and that may be a bit too strict a filter (which this
> series takes us in a good direction to fix) but what the readers
> need to know is the real-world implications of the choices made at
> the implementation detail level, and more importantly, what the
> implementation is trying to compute.
> 
> It is a documentation bug (with or without these patches) if the
> current text gives an impression that the code is trying to do
> anything but "guessing the fork point using historical tips".

I'm still trying to understand what the original intent was: If we
abstract from the implementation (as we should, as you rightly
emphasize) and talk about historical tips then we have to ask ourselves:
- What is "historical"?
- What is tip?
- Tip of what, i.e. what is a "branch"?

If by "branch" we mean the moving branch ref that it is in git then by
all means, historical tips are the values that that ref ever had, and
all that we can say is that this includes the current value and the
current contents of the branch refs's reflog (which may or may not be
"complete").

Note that this notion of "branch" is completely independent of the DAG,
whereas by definition a "merge-base" is a concept that relies on an
ancestry graph.

If by "branch" we mean everything that is "on" (or in, think
"--contains") that branch - and I assume that is how most users think
about a branch - then it is not clear at all why we should focus on
"historical values that that refname had", which is an implemenation
detail in itself (branch refs is how we implement the branch concept).

Especially, it's not clear why we should exclude for example a commit
that is in between two commits that are in the reflog ("historical
tips") of a branch that has been fast forwarded or reset (-A, then fast
forward to -A-B-C; this excludes B from the list of merge-base candidates).

>> In any case, for two modes we need two names for the options. Maybe
>> --fork-point and --fork-base because in the loose mode, you may get a
>> "base of a strict fork point"?
> 
> Perhaps.
> 

Re: [PATCH 2/3] merge-base: return fork-point outside reflog

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 21.09.2017 08:27:
> Junio C Hamano  writes:
> 
>> ...  I agree that there is a value in what your patch 2/3
>> wants to do when the current one that is more strict would say
>> "there is no known fork-point"---we would gain a way to say "... but
>> this is the best guess based on available data that may be better
>> than getting no answer." which we lack.
>>
>> Having said all that, I do not agree with your last sentence in the
>> paragraph I quoted above.  It is a mere implementation detail to
>> consult the reflog to find out the set of "historical tips of the
>> Branch"; the current tip by definition is among the commits in that
>> set, even when the reflog of Branch is missing.  What 4f21454b55 did
>> was a reasonable "fix" that is still in line with the definition of
>> "--fork-point" from that point of view.
>>
>> Whether we add a "looser" version of "--fork-point" to the system or
>> not, the more strict version should still use the current tip as one
>> of the historical tips (i.e. those that we would take from the
>> reflog if the reflog were not empty) in the more "strict" mode.  The
>> looser version may also want to do so as well.
> 
> So, should I mark this in What's cooking report as "expecting a
> reroll", anticipating that a new option would be added to trigger
> the new & looser behaviour?
> 

I dunno. Some participants in this thread considered my patch to be a
fix rather than alternative behaviour. So I hoped for more responses to
your response. (Re-adding dscho on cc - our thread graph forked...)

Also, I'm undecided about about your reflog argument above - if we leave
"--fork-point" to be the current behaviour including Jeff's fix then the
documentation would need an even bigger overhaul, because it's neither
"reflog also" (as claimed in the doc) nor "reflog only" (as in the
original implementation) but "historical tips as inferred from the
current value and the reflog".

In any case, for two modes we need two names for the options. Maybe
--fork-point and --fork-base because in the loose mode, you may get a
"base of a strict fork point"?

Michael

Re: [PATCH 1/2] test-lib: group system specific FIFO tests by system

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 15.09.2017 00:21:
> Hi Michael,
> 
> On Thu, 14 Sep 2017, Michael J Gruber wrote:
> 
>> test-lib determines whether a file-system supports FIFOs and needs to do
>> special casing for CYGWIN and MINGW. This separates those system
>> specific settings from those at more central place.
>>
>> Set mkfifo()  to false in the central system specific place so that the
>> same test works everywhere.
> 
> The mkfifo() emulation of Cygwin seems to work, no? I think it works even
> in MSYS2, but not in MINGW.
> 
> So maybe this patch should affect only the MINGW arm?

I only reorganised the code, so in that sense the patch does not affect
any system ;)

If indeed mkfifo works on CYGWIN than a separate patch should remove the
exclusion of CYGWIN; alas, I can't confirm (I wish MS still had the old
academic alliance programme).

Michael

Re: [PATCH 2/3] merge-base: return fork-point outside reflog

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 15.09.2017 04:48:
> Michael J Gruber <g...@grubix.eu> writes:
> 
>> In fact, per documentation "--fork-point" looks at the reflog in
>> addition to doing the usual walk from the tip. The original design
>> description in d96855ff51 ("merge-base: teach "--fork-point" mode",
>> 2013-10-23) describes this as computing from a virtual merge-base of all
>> the historical tips of refname. They may or may not all be present in
>> the reflog (think pruning, non-ff fetching, fast forwarding etc.),
>> so filtering by the current contents of the reflog is potentially
>> harmful, and it does not seem to fulfill any purpose in the original
>> design.
> 
> Let me think aloud, using the picture from the log message from that
> commit.
> 
>  o---B1
> /
> ---o---o---B2--o---o---o---Base
> \
>  B3
>   \
>Derived
> 
> where the current tip of the "base" branch is at Base, but earlier
> fetch observed that its tip used to be B3 and then B2 and then B1
> before getting to the current commit, and the branch being rebased
> on top of the latest "base" is based on commit B3.
> 
> So the logic tries to find a merge base between "Derived" and a
> virtual merge commit across Base, B1, B2 and B3.  And it finds B3.
> 
> If for some reason we didn't have B3 in the reflog, then wouldn't
> the merge base computation between Derived and a virtual merge
> commit across Base, B2 and B2 (but not B3 because we no longer know
> about it due to its lack in the reflog) find 'o' that is the parent
> of B2 and B3? 

Yes.

> Wouldn't that lead to both B3 and Derived replayed
> when the user of the fork-point potion rebases the history of
> Derived?

Replayed, yes. What that means would depend on how B3 ended up being
"off base" (reset or rebase, e.g.): the replay could lead to a reapply
without conflict, or with conflict, or an empty (discarded) commit,
depending on "how much of B3" is still "on base".

> Perhaps that is the best we could do with a pruned reflog that lacks
> B3, but if that is the case, I wonder if it may be better to fail
> the request saying that we cannot find the fork-point (because,
> well, your reflog no longer has sufficient information), than
> silently give a wrong result, and in this case, we can tell between
> a correct result (i.e. the merge base is one of the commits we still
> know was at the tip) and a wrong one (i.e. the merge base is not any
> of the commits in the reflog).
> 
> If we declare --fork-point is the best effort option and may give an
> answer that is not better than without the option, then I think this
> patch is OK, but that diminishes the value of the option as a
> building block, I am afraid.
> 
> Callers that are more careful could ask merge-base with --fork-point
> (and happily use it knowing that the result is indeed a commit that
> used to be at the tip), or fall back to the result merge-base
> without --fork-point gives (because you could do no better) and deal
> with duplicates that may happen due to the imprecise determination.
> With this change, these callers will get result from a call to
> "merge-base --fork-point" that may or may not be definite, and they
> cannot tell.  For lazy users, making the option itself to fall back
> may be simpler to use, and certainly is a valid stance to take when
> implementing a convenience option to a Porcelain command, but I do
> not know if it is a good idea to throw "merge-base --fork-point"
> into that category.

Simply put, "git merge-base ref commit" looks at the (graph) history of
ref and considers merge-base candidates that are also in the graph
history of commit. This is the "graph notion" of merge-base, and the
result is immanently determined by the DAG.

There is also the "reflog notion" where you look at the "reflog history"
of ref. The result depends on the reflog, which itself is "volatile"
(think prune), and such is the result.

Now, the original documentation of merge-base says that "merge-base
--fork-point" looks at the reflog of ref "also" (*in addition to*) the
DAG. That is, the merge-base candidates for "merge-base --fork-point"
are a super-set of those for the plain mode, enhanced by the reflog.
(graph notion plus reflog notion)

The original implementation makes sure that "merge-base --fork-point"
looks *only* at candidates from the reflog. (strict reflog notion)

That is a discrepancy that we should resolve in any case.

Note tha

Re: [PATCH 1/3] t6010: test actual test output

[Michael J Gruber]

Jeff King venit, vidit, dixit 14.09.2017 16:34:
> On Thu, Sep 14, 2017 at 03:15:18PM +0200, Michael J Gruber wrote:
> 
>> 4f21454b55 ("merge-base: handle --fork-point without reflog",
>> 2016-10-12) introduced a fix for merge-base --fork-point without reflog
>> and a test. While that test is fine, it did not update expected nor actual
>> output and tested that of the previous test instead.
> 
> Oops. The use of the existing "expect3" was intentional (the idea being
> that we are repeating the identical step from the previous test with the
> slight tweak of not having a reflog). But obviously failing to write to
> "actual" at all was a mistake.
> 
> That said, I'm OK with reiterating the expected output.

expect3 had a different content, too ;)

Michael

Re: [PATCH v3] commit-template: change a message to be more intuitive

[Michael J Gruber]

Kaartic Sivaraam venit, vidit, dixit 15.09.2017 06:50:
> It's not good to use the phrase 'do not touch' to convey the information
> that the cut-line should not be modified or removed as it could possibly
> be mis-interpreted by a person who doesn't know that the word 'touch' has
> the meaning of 'tamper with'. Further, it could make translations a little
> difficult as it might not have the intended meaning in a few languages (for
> which translations don't exist yet) when translated as such.
> 
> So, use intuitive terms in the sentence. Replacing the word 'touch' with
> other terms has introduced the possibility of the following sentence to be
> mis-interpreted, so change the terms in that too.
> 
> Signed-off-by: Kaartic Sivaraam 
> ---
>  Changes in v3:
>  
> - changed the wordings of the second sentence as there seemed to be a
>   magical 'or else' connecting the two lines.
>  
>  I didn't expect the least that this would go upto v3. In case anyboy finds
>  something wrong with this change too, it's a lot better to drop this 
> altogether
>  than going for a v4.

That happens more often than not :)

Your original intent was to avoid any misunderstandings, and all the
comments and iterations made sure that we don't trade one possible
source of misunderstanding for another but avoid them all.

I consider v4 to be a strict improvement over the status quo and (as fas
as I can see) to serve your original intent as good as possible.

>  wt-status.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/wt-status.c b/wt-status.c
> index 77c27c511..23e87e74d 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -934,7 +934,7 @@ size_t wt_status_locate_end(const char *s, size_t len)
>  
>  void wt_status_add_cut_line(FILE *fp)
>  {
> - const char *explanation = _("Do not touch the line above.\nEverything 
> below will be removed.");
> + const char *explanation = _("Do not modify or remove the line 
> above.\nEverything below it will be ignored.");
>   struct strbuf buf = STRBUF_INIT;
>  
>   fprintf(fp, "%c %s", comment_line_char, cut_line);
> 

[PATCH 2/2] test-lib: ulimit does not limit on CYGWIN and MINGW

[Michael J Gruber]

ulimit succeeds (by return value) but does not limit on some systems.

Set ulimit() to false on these systems so that we do not rely on its
output nor effect. As an intended side-effect, ulimit based
prerequisites are set correctly (to "not-have") on these systems.

Reported-by: Ramsay Jones <ram...@ramsayjones.plus.com>
Reported-by: Adam Dinwoodie <a...@dinwoodie.org>
Reported-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
This is independent of my series, but should best go before so that no
ulimit based test is run on CYGWIN and MINGW.

It follows the basic assumption that a tool like ulimit is either
present and functional or not present; and that we work around by
defines or such when that assumption is broken.
(Alternatively, we could set ULIMT_LIMITS or so and depend on that.)

 t/test-lib.sh | 8 
 1 file changed, 8 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index b8a0b05102..f6ac60d487 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -998,6 +998,10 @@ case $uname_s in
mkfifo () {
false
}
+   # ulimit succeeds but does not limit
+   ulimit () {
+   false
+   }
# no POSIX permissions
# backslashes in pathspec are converted to '/'
# exec does not inherit the PID
@@ -1012,6 +1016,10 @@ case $uname_s in
mkfifo () {
false
}
+   # ulimit succeeds but does not limit
+   ulimit () {
+   false
+   }
test_set_prereq POSIXPERM
test_set_prereq EXECKEEPSPID
test_set_prereq CYGWIN
-- 
2.14.1.712.gda4591c8a2

[PATCH 1/2] test-lib: group system specific FIFO tests by system

[Michael J Gruber]

test-lib determines whether a file-system supports FIFOs and needs to do
special casing for CYGWIN and MINGW. This separates those system
specific settings from those at more central place.

Set mkfifo()  to false in the central system specific place so that the
same test works everywhere.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 t/test-lib.sh | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5fbd8d4a90..b8a0b05102 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -994,6 +994,10 @@ case $uname_s in
pwd () {
builtin pwd -W
}
+   # no FIFOs
+   mkfifo () {
+   false
+   }
# no POSIX permissions
# backslashes in pathspec are converted to '/'
# exec does not inherit the PID
@@ -1004,6 +1008,10 @@ case $uname_s in
GIT_TEST_CMP=mingw_test_cmp
;;
 *CYGWIN*)
+   # no FIFOs
+   mkfifo () {
+   false
+   }
test_set_prereq POSIXPERM
test_set_prereq EXECKEEPSPID
test_set_prereq CYGWIN
@@ -1062,14 +1070,7 @@ test_i18ngrep () {
 
 test_lazy_prereq PIPE '
# test whether the filesystem supports FIFOs
-   case $(uname -s) in
-   CYGWIN*|MINGW*)
-   false
-   ;;
-   *)
-   rm -f testfifo && mkfifo testfifo
-   ;;
-   esac
+   rm -f testfifo && mkfifo testfifo
 '
 
 test_lazy_prereq SYMLINKS '
-- 
2.14.1.712.gda4591c8a2

[PATCH 0/3] merge-base --fork-point fixes

[Michael J Gruber]

merge-base --fork-point does not quite work as advertised when the
reflog is empty or partial. This series brings it in line with the
documentation and, hopefully, with the original intent.

Michael J Gruber (3):
  t6010: test actual test output
  merge-base: return fork-point outside reflog
  merge-base: find fork-point outside partial reflog

 builtin/merge-base.c  | 20 
 t/t6010-merge-base.sh | 21 +++--
 2 files changed, 23 insertions(+), 18 deletions(-)

-- 
2.14.1.712.gda4591c8a2

[PATCH 1/3] t6010: test actual test output

[Michael J Gruber]

4f21454b55 ("merge-base: handle --fork-point without reflog",
2016-10-12) introduced a fix for merge-base --fork-point without reflog
and a test. While that test is fine, it did not update expected nor actual
output and tested that of the previous test instead.

Fix the test to test the merge-base output, not just its return code.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 t/t6010-merge-base.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
index 31db7b5f91..17fffd7998 100755
--- a/t/t6010-merge-base.sh
+++ b/t/t6010-merge-base.sh
@@ -262,8 +262,9 @@ test_expect_success 'using reflog to find the fork point' '
 
 test_expect_success '--fork-point works with empty reflog' '
git -c core.logallrefupdates=false branch no-reflog base &&
-   git merge-base --fork-point no-reflog derived &&
-   test_cmp expect3 actual
+   git rev-parse base >expect &&
+   git merge-base --fork-point no-reflog derived >actual &&
+   test_cmp expect actual
 '
 
 test_expect_success 'merge-base --octopus --all for complex tree' '
-- 
2.14.1.712.gda4591c8a2

[PATCH 3/3] merge-base: find fork-point outside partial reflog

[Michael J Gruber]

In fork-point mode, merge-base adds the commit at refname to a list of
candidates to walk from only when the reflog is empty. Therefore, it
fails to find merge bases that are descendants of the most recent reflog
entry.

Add the commit at the tip unconditionally so that a merge base on the
history of refname is found in any case, independent of the state of the
reflog.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/merge-base.c  | 2 +-
 t/t6010-merge-base.sh | 8 
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index 926a7615ea..b5b4cf7eac 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -174,7 +174,7 @@ static int handle_fork_point(int argc, const char **argv)
revs.initial = 1;
for_each_reflog_ent(refname, collect_one_reflog_ent, );
 
-   if (!revs.nr && !get_oid(refname, ))
+   if (!get_oid(refname, ))
add_one_commit(, );
 
for (i = 0; i < revs.nr; i++)
diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
index 850463d4f2..78342896c7 100755
--- a/t/t6010-merge-base.sh
+++ b/t/t6010-merge-base.sh
@@ -275,6 +275,14 @@ test_expect_success '--fork-point works with merge-base 
outside reflog' '
test_cmp expect actual
 '
 
+test_expect_success '--fork-point works with merge-base outside partial 
reflog' '
+   git -c core.logallrefupdates=true branch partial-reflog base &&
+   git rev-parse no-reflog >.git/refs/heads/partial-reflog &&
+   git rev-parse no-reflog >expect &&
+   git merge-base --fork-point partial-reflog no-reflog >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'merge-base --octopus --all for complex tree' '
# Best common ancestor for JE, JAA and JDD is JC
# JE
-- 
2.14.1.712.gda4591c8a2

[PATCH 2/3] merge-base: return fork-point outside reflog

[Michael J Gruber]

4f21454b55 ("merge-base: handle --fork-point without reflog",
2016-10-12) fixed the case without reflog, but only partially:
The original code checks whether the merge base candidates are from the
list that we started with, which was the list of reflog entries before
4f21454b55 and the list of reflog entries - or the ref itself if empty -
after. The test from 4f21454b55 tested in a situation where the merge
base candidate equalled the commit at refname, so it was on this (1
item) list by accident.

In fact, per documentation "--fork-point" looks at the reflog in
addition to doing the usual walk from the tip. The original design
description in d96855ff51 ("merge-base: teach "--fork-point" mode",
2013-10-23) describes this as computing from a virtual merge-base of all
the historical tips of refname. They may or may not all be present in
the reflog (think pruning, non-ff fetching, fast forwarding etc.),
so filtering by the current contents of the reflog is potentially
harmful, and it does not seem to fulfill any purpose in the original
design.

Remove the filtering and add a test for an out-of-reflog merge base.

Reported-by: Ekelhart Jakob <jakob.ekelh...@fsw.at>
Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/merge-base.c  | 18 +++---
 t/t6010-merge-base.sh |  8 
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index 6dbd167d3b..926a7615ea 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -186,23 +186,11 @@ static int handle_fork_point(int argc, const char **argv)
 * There should be one and only one merge base, when we found
 * a common ancestor among reflog entries.
 */
-   if (!bases || bases->next) {
+   if (!bases || bases->next)
ret = 1;
-   goto cleanup_return;
-   }
-
-   /* And the found one must be one of the reflog entries */
-   for (i = 0; i < revs.nr; i++)
-   if (>item->object == [i]->object)
-   break; /* found */
-   if (revs.nr <= i) {
-   ret = 1; /* not found */
-   goto cleanup_return;
-   }
-
-   printf("%s\n", oid_to_hex(>item->object.oid));
+   else
+   printf("%s\n", oid_to_hex(>item->object.oid));
 
-cleanup_return:
free_commit_list(bases);
return ret;
 }
diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
index 17fffd7998..850463d4f2 100755
--- a/t/t6010-merge-base.sh
+++ b/t/t6010-merge-base.sh
@@ -267,6 +267,14 @@ test_expect_success '--fork-point works with empty reflog' 
'
test_cmp expect actual
 '
 
+test_expect_success '--fork-point works with merge-base outside reflog' '
+   git -c core.logallrefupdates=false checkout no-reflog &&
+   git -c core.logallrefupdates=false commit --allow-empty -m "Commit 
outside reflogs" &&
+   git rev-parse base >expect &&
+   git merge-base --fork-point no-reflog derived >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'merge-base --octopus --all for complex tree' '
# Best common ancestor for JE, JAA and JDD is JC
# JE
-- 
2.14.1.712.gda4591c8a2

Re: [PATCH] test-lib: don't use ulimit in test prerequisites on cygwin

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 13.09.2017 21:20:
> Ramsay Jones wrote:
> 
>> On cygwin (and MinGW), the 'ulimit' built-in bash command does not have
>> the desired effect of limiting the resources of new processes, at least
>> for the stack and file descriptors. However, it always returns success
>> and leads to several test prerequisites being erroneously set to true.
>>
>> Add a check for cygwin and MinGW to the prerequisite expressions, using
>> 'uname -s', and return false instead of (indirectly) using 'ulimit'.
>> This affects the prerequisite expressions for the ULIMIT_STACK_SIZE,
>> CMDLINE_LIMIT and ULIMIT_FILE_DESCRIPTORS prerequisites.
>>
>> Signed-off-by: Ramsay Jones 
>> ---
>>  t/t1400-update-ref.sh | 11 ++-
>>  t/t6120-describe.sh   |  1 -
>>  t/t7004-tag.sh|  1 -
>>  t/test-lib.sh | 22 --
>>  4 files changed, 30 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Jonathan Nieder 
> 
> Thanks.
> 
> An alternative would be to do some more explicit feature-based test
> like using "ulimit" to set a limit and then reading back the limit in
> a separate process, but that feels like overkill.

It's still not clear whether these limits would be in effect or just
reported.

Rather than checking uname -s in several places, I think we should just
define ulimit to be false in one place, or rather set the prerequisite
there.

Michael

Re: merge-base not working as expected when base is ahead

[Michael J Gruber]

Ekelhart Jakob venit, vidit, dixit 13.09.2017 17:07:
> Dear Git,
> 
> git merge-base --fork-point "master" not working if master is already newer 
> then my current branch.
> Very oddly it seems to work whenever you had the expected commit checked out 
> previously - what made it very tricky to detect this problem.
> 
> Example:
>  - Clone "https://github.com/jekelhart/GitInfoTry;
>  - Switch to branch "v1.0.0"
>  - git merge-base --fork-point "master"
>- or: git merge-base --fork-point "origin/master" 
>  - expected result: fork point "fbb1db34c6317a6e8b319c1ec261e97ca1672c22"
>  - but result is empty
> 
> In the repo where we created this example tree in the first place the command 
> returned the expected fork point. If you clone it new and fresh it does not 
> return any result anymore.
> 
> Works, however, on branch "v2.0.0". Assumption: because "master" is older.(?)
> I think it works locally because the command uses the reflog in addition(!), 
> however, it should work without the local reflog as well. (since the history 
> was not modified in any way)

The documentation lies, unfortunately. It claims that in fork-mode, "git
merge-base" "also" looks at the reflog. In fact, the code explicitely
*discards* any merge bases that it finds but which are not in the
reflog. (The merge base that you find for v2.0.0 is in the reflog
because it's the checked out HEAD.)

Removing the corresponding check gives the merge base for v1.0.0 that
you expect, and git still passes all tests. But I'll look at the history
of these lines before I submit a patch.

Michael

Re: [PATCH v2] commit-template: change a message to be more intuitive

[Michael J Gruber]

Kaartic Sivaraam venit, vidit, dixit 13.09.2017 15:05:
> It's not good to use the phrase 'do not touch' to convey the information
> that the cut-line should not be modified or removed as it could possibly
> be mis-interpreted by a person who doesn't know that the word 'touch' has
> the meaning of 'tamper with'. Further, it could make translations a little
> difficult as it might not have the intended meaning in a few languages when
> translated as such.
> 
> So, use more intuitive terms in the sentence.
> 
> Signed-off-by: Kaartic Sivaraam 
> ---
>  wt-status.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/wt-status.c b/wt-status.c
> index 77c27c51134d2..be53579760ee7 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -934,7 +934,7 @@ size_t wt_status_locate_end(const char *s, size_t len)
>  
>  void wt_status_add_cut_line(FILE *fp)
>  {
> - const char *explanation = _("Do not touch the line above.\nEverything 
> below will be removed.");
> + const char *explanation = _("Do not modify or remove the line 
> above.\nEverything below will be removed.");

I don't want to complicate things. But now - due to the repeated usage
of "remove" - these two sentences seem to be connected by an invisible
"or else" ("will" vs. "would" not withstanding). i.e. "or else
everything below will be removed, too", which is wrong. Before, they
were separated more clearly.

Also, given all the translations that we have, it seems somewhat strange
to try and foresee and workaround possible misunderstandings of commonly
used English phrases.

>   struct strbuf buf = STRBUF_INIT;
>  
>   fprintf(fp, "%c %s", comment_line_char, cut_line);
> 
> --
> https://github.com/git/git/pull/401
> 

Re: Unexpected pass for t6120-describe.sh on cygwin

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 12.09.2017 15:39:
> Hi Ramsay,
> 
> On Sat, 9 Sep 2017, Ramsay Jones wrote:
> 
>> I ran the test-suite on the 'pu' branch last night (simply because that
>> was what I had built at the time!), which resulted in a PASS, but t6120
>> was showing a 'TODO passed' for #52.
>>
>> This is a test introduced by Michael's 'mg/name-rev-tests-with-short-stack'
>> branch, which uses 'ulimit -s' to try and force a stack overflow.
>> Unfortunately, 'ulimit -s' seems to have no effect on cygwin. I created
>> a test program (see below) to eat up the stack and tried running it with
>> various ulimit values (128, 12, 8), but it always seg-faulted at the
>> same stack-frame. (after using approx 2MB stack space).
>>
>> So, it looks like all ULIMIT_STACK_SIZE tests need to be disabled
>> on cygwin. I also wonder about the ULIMIT_FILE_DESCRIPTORS tests, but
>> haven't looked into it.
>>
>> Given that 'ulimit' is a bash built-in, this may also be a problem on
>> MinGW and Git-For-Windows, but I can't test on those platforms.
> 
> It is.

Thanks.

I just dug something up from an old cygwin list post. Could you please
try and report on the following (cygwin, MinGW):

ulimit -s
ulimit -s 4096 # anything lower than the value from above
ulimit -s
bash -c "ulimit -s"

My suspicion from that post is that ulimit affects the sub shell only -
if yes, running a test inside the sub shell to confirm whether the
setting is in effect would be great, of course. /usr/bin/echo $(seq
10) or such does the job on Linux (with stack limit 128), but I
don't know whether this is portably reliable.

If ulimit on these platforms has no effect but does not lie about the
value either it's a simple prerequisite test (test ulimit present, if
yes set and get and compare the stack size values).

Michael

Re: Unexpected pass for t6120-describe.sh on cygwin

[Michael J Gruber]

Ramsay Jones venit, vidit, dixit 09.09.2017 15:13:
> Hi Adam,
> 
> I ran the test-suite on the 'pu' branch last night (simply because
> that was what I had built at the time!), which resulted in a PASS,
> but t6120 was showing a 'TODO passed' for #52.
> 
> This is a test introduced by Michael's 'mg/name-rev-tests-with-short-stack'
> branch, which uses 'ulimit -s' to try and force a stack overflow.
> Unfortunately, 'ulimit -s' seems to have no effect on cygwin. I created
> a test program (see below) to eat up the stack and tried running it
> with various ulimit values (128, 12, 8), but it always seg-faulted
> at the same stack-frame. (after using approx 2MB stack space).
> 
> So, it looks like all ULIMIT_STACK_SIZE tests need to be disabled
> on cygwin. I also wonder about the ULIMIT_FILE_DESCRIPTORS tests,
> but haven't looked into it.
> 
> Given that 'ulimit' is a bash built-in, this may also be a problem
> on MinGW and Git-For-Windows, but I can't test on those platforms.
> 
> Unfortunately, I can't spend more time on git today, hence this
> heads up! ;-)

Thanks for the note. We have this in t/test-lib.sh:

run_with_limited_cmdline () {
(ulimit -s 128 && "$@")
}

test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'

This apparantly expects "ulimit -s" to fail on platforms that don't
support it, so set the prereq accordingly. I moved the following to
t/test-lib.sh:

run_with_limited_stack () {
(ulimit -s 128 && "$@")
}

test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'

Same things as above. Two things to note:
- Those requisites could be the same, also they are used in different ways.
- "ulimit -s" returning success without doing anything means that, all
along, we ran the existing tests when we didn't mean to (on Win), and
they succeeded for the wrong reason, which we did not notice.

So, I guess, short of testing the effect of "ulimit -s" with another
expensive test, it's best to simply set these prerequisites based on
"uname -s".


> ATB,
> Ramsay Jones
> 
> -- >8 --
> diff --git a/test.c b/test.c
> new file mode 100644
> index 000..bcbb805
> --- /dev/null
> +++ b/test.c
> @@ -0,0 +1,21 @@
> +#include 
> +#include 
> +#include 
> +
> +void test(uint64_t count)
> +{
> + int i, junk[1024];
> +
> + for (i = 0; i < 1024; i++)
> + junk[i] = count;
> + i = junk[count % 1024];
> + printf("%" PRIuMAX "\n", (uintmax_t)count);
> + fflush(stdout);
> + test(count + 1);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + test(0);
> + return 0;
> +}
> 

Re: git diff doesn't quite work as documented?

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 08.09.2017 03:26:
> Olaf Klischat  writes:
> 
>> `git diff --help' says:
>>
>> git diff [--options]  [--] [...]
>>This form is to view the changes you have in your
>>working tree relative to the named .
> 
> That help text is poorly phrased.  
> 
> When "git diff" talks about files in your working tree, it always
> looks them _through_ the index.  As far as the command is concerned,
> a cruft left in your working tree that is not in the index does
> *not* exist.
> 
> So when your index does not have bar.txt, even if you have an
> untracked bar.txt in your directory, i.e.
> 
>> oklischat@oklischat:/tmp/gittest$ git status
>> On branch master
>> Untracked files:
>>   (use "git add ..." to include in what will be committed)
>>
>>  bar.txt
> 
> and you have a commit that _has_ that file, then the command thinks
>  has the path, and your working tree does *not*.  IOW, this
> is...
> 
>> oklischat@oklischat:/tmp/gittest$ git diff bar-added
>> diff --git a/bar.txt b/bar.txt
>> deleted file mode 100644
> 
> ... totally expected and intended output.
> 
> Hope the above explanation clarifies.  A documentation update might
> be helpful to new users.

Well, there's a difference between "working tree" and  "working dir".
The wt is "the tree of actual checked out files" per our glossary. So
maybe the doc could point to the glossary (see the glossary for the
difference to the work dir).

But really, this type of misunderstandings comes up often: people try to
understand the doc based on common language terms (which is okay, of
course) and are unaware of the defined meanings of technical terms.
Explaining them in every place where they are used simply does not scale.

Maybe we should make more use of our glossary (extend it, enhance it,
promote it) and somehow mark all technical terms as such when they are
used (say, italics in HTML), or at least when the exact meaning is
relevant like in the case above, and possibly link to the glossary (-item).

Michael

Re: [PATCH 0/4] Test name-rev with small stack

[Michael J Gruber]

Jeff King venit, vidit, dixit 07.09.2017 16:54:
> On Thu, Sep 07, 2017 at 04:02:19PM +0200, Michael J Gruber wrote:
> 
>> name-rev segfaults for me in emacs.git with the typical 8102 stack size.
>> The reason is the recursive walk that name-rev uses.
>>
>> This series adds a test to mark this as known failure, after some
>> clean-ups.
> 
> These all look reasonable to me. The size of the test case in the final
> one is presumably arbitrary and just copied from t7004. I don't know if
> it's worth trying to shrink it. It could shorten a rather expensive
> test. OTOH, if we shorten it too much then we might get a false pass
> (e.g., if the algorithm remains recursive but has a smaller stack
> footprint).
> 
>> Michael J Gruber (4):
>>   t7004: move limited stack prereq to test-lib
>>   t6120: test name-rev --all and --stdin
>>   t6120: clean up state after breaking repo
>>   t6120: test describe and name-rev with deep repos
> 
> Now comes the hard part: rewriting the C code. :)

Looking at it more closely, the solution in cbc60b6720 ("git tag
--contains: avoid stack overflow", 2014-04-24) seems to be a bit "ad
hoc" to me:

First of all, there is more than "tag --contains" that may exceed the
stack by recursion. So I would expect the solution to be more general,
and not localised and specialised to builtin/tag.c

Second, this is a walk, so I'm wondering whether our revision walk
machinery should be the place to add missing functionality (if any).
That way, everything would benefit from possible or existing
improvements there. For example, I think some of our "extra walkers"
don't heed object replacements. (I need to test more.)

Michael

[PATCH 0/4] Test name-rev with small stack

[Michael J Gruber]

name-rev segfaults for me in emacs.git with the typical 8102 stack size.
The reason is the recursive walk that name-rev uses.

This series adds a test to mark this as known failure, after some
clean-ups.

Michael J Gruber (4):
  t7004: move limited stack prereq to test-lib
  t6120: test name-rev --all and --stdin
  t6120: clean up state after breaking repo
  t6120: test describe and name-rev with deep repos

 t/t6120-describe.sh | 57 +
 t/t7004-tag.sh  |  6 --
 t/test-lib.sh   |  6 ++
 3 files changed, 63 insertions(+), 6 deletions(-)

-- 
2.14.1.603.gf58147c36e

[PATCH 1/4] t7004: move limited stack prereq to test-lib

[Michael J Gruber]

The lazy prerequisite  ULIMIT_STACK_SIZE is used only in t7004 so far.

Move it to test-lib.sh so that it can be used in other tests (which it will
be in a follow-up commit).

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 t/t7004-tag.sh | 6 --
 t/test-lib.sh  | 6 ++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index dbcd6f623c..5bf5ace56b 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1863,12 +1863,6 @@ test_expect_success 'version sort with very long 
prerelease suffix' '
git tag -l --sort=version:refname
 '
 
-run_with_limited_stack () {
-   (ulimit -s 128 && "$@")
-}
-
-test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
-
 # we require ulimit, this excludes Windows
 test_expect_success ULIMIT_STACK_SIZE '--contains and --no-contains work in a 
deep repo' '
>expect &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5fbd8d4a90..f22c1b260a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1167,6 +1167,12 @@ run_with_limited_cmdline () {
 
 test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'
 
+run_with_limited_stack () {
+   (ulimit -s 128 && "$@")
+}
+
+test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
+
 build_option () {
git version --build-options |
sed -ne "s/^$1: //p"
-- 
2.14.1.603.gf58147c36e

[PATCH 2/4] t6120: test name-rev --all and --stdin

[Michael J Gruber]

name-rev is used in a few tests, but tested only in t6120 along with
describe so far.

Add tests for name-rev with --all and --stdin.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 t/t6120-describe.sh | 25 +
 1 file changed, 25 insertions(+)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index aa74eb8f0d..7c5728ebd5 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -198,6 +198,31 @@ test_expect_success 'name-rev with exact tags' '
test_cmp expect actual
 '
 
+test_expect_success 'name-rev --all' '
+   >expect.unsorted &&
+   for rev in $(git rev-list --all)
+   do
+   git name-rev $rev >>expect.unsorted
+   done &&
+   sort expect &&
+   git name-rev --all >actual.unsorted &&
+   sort actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'name-rev --stdin' '
+   >expect.unsorted &&
+   for rev in $(git rev-list --all)
+   do
+   name=$(git name-rev --name-only $rev) &&
+   echo "$rev ($name)" >>expect.unsorted
+   done &&
+   sort expect &&
+   git rev-list --all | git name-rev --stdin >actual.unsorted &&
+   sort actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'describe --contains with the exact tags' '
echo "A^0" >expect &&
tag_object=$(git rev-parse refs/tags/A) &&
-- 
2.14.1.603.gf58147c36e

[PATCH 4/4] t6120: test describe and name-rev with deep repos

[Michael J Gruber]

Depending on the implementation of walks, limitted stack size may lead
to problems (for recursion).

Test name-rev and describe with deep repos and limitted stack size and
mark the former with known failure.

We add these tests (which add gazillions of commits) last so as to keep
the runtime of other subtests the same.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 t/t6120-describe.sh | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 1997ccde56..dd6dd9df9b 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -279,4 +279,35 @@ test_expect_success 'describe ignoring a borken submodule' 
'
grep broken out
 '
 
+# we require ulimit, this excludes Windows
+test_expect_failure ULIMIT_STACK_SIZE 'name-rev works in a deep repo' '
+   i=1 &&
+   while test $i -lt 8000
+   do
+   echo "commit refs/heads/master
+committer A U Thor <aut...@example.com> $((10 + $i * 100)) +0200
+data <expect &&
+   git name-rev HEAD~4000 >actual &&
+   test_cmp expect actual &&
+   run_with_limited_stack git name-rev HEAD~4000 >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success ULIMIT_STACK_SIZE 'describe works in a deep repo' '
+   git tag -f far-far-away HEAD~7999 &&
+   echo "far-far-away" >expect &&
+   git describe --tags --abbrev=0 HEAD~4000 >actual &&
+   test_cmp expect actual &&
+   run_with_limited_stack git describe --tags --abbrev=0 HEAD~4000 >actual 
&&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.14.1.603.gf58147c36e

[PATCH 3/4] t6120: clean up state after breaking repo

[Michael J Gruber]

t6120 breaks the repo state intentionally in the last tests.

Clean up the breakage afterwards (and before adding more tests).

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 t/t6120-describe.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 7c5728ebd5..1997ccde56 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -275,6 +275,7 @@ test_expect_success 'describe chokes on severely broken 
submodules' '
 '
 test_expect_success 'describe ignoring a borken submodule' '
git describe --broken >out &&
+   test_when_finished "mv .git/modules/sub_moved .git/modules/sub1" &&
grep broken out
 '
 
-- 
2.14.1.603.gf58147c36e

Re: [PATCH] name-rev: change ULONG_MAX to TIME_MAX

[Michael J Gruber]

Jeff King venit, vidit, dixit 06.09.2017 15:35:
> On Wed, Sep 06, 2017 at 01:59:31PM +0200, Michael J Gruber wrote:
> 
>> BTW, there's more fallout from those name-rev changes: In connection
>> with that other thread about surprising describe results for emacs.git I
>> noticed that I can easily get "git name-rev --stdin" to segfault there.
>> As easy as
>>
>> echo bc5d96a0b2a1dccf7c459e40d21b54c977f4 | git name-rev --stdin
>>
>> for example.
>>
>> That's unfortunate for the use-case of name-rev to amend git log output.
>>
>> The reason seems to be that with "--stdin" or "--all", "name-rev" walks
>> and names all commits before beginning to use that those names for even
>> a single commit as above.
>>
>> That segfault bisects to the logic changing commit in
>> jc/name-rev-lw-tag, but I think the changed logic simply leads to more
>> xmallocs() the segfault sooner now. Or something that I dind't spot even
>> after a few hours.
> 
> The segfault seems to be due to running out of stack space. The problem
> is that name_rev() is recursive over the history graph.  That topic
> added a parameter to the function, which increased the memory used for
> each level of the recursion. But the fundamental problem has always been
> there. The right solution is to switch to iteration (with our own stack
> structure if necessary).
> 
> We had similar problems with the recursive --contains traversal in tag,
> and ended up with cbc60b6720 (git tag --contains: avoid stack overflow,
> 2014-04-24).

Cool, thanks for the pointer. ulimit -s is a great way to test this.

Michael

Re: [PATCH] name-rev: change ULONG_MAX to TIME_MAX

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 06.09.2017 05:35:
> Michael J Gruber <g...@grubix.eu> writes:
> 
>> Earlier, dddbad728c ("timestamp_t: a new data type for timestamps",
>> 2017-04-26) changed several types to timestamp_t.
>>
>> 5589e87fd8 ("name-rev: change a "long" variable to timestamp_t",
>> 2017-05-20) cleaned up a missed variable, but both missed a _MAX
>> constant.
>>
>> Change the remaining constant to the one appropriate for the current
>> type
>>
>> Signed-off-by: Michael J Gruber <g...@grubix.eu>
>> ---
> 
> Thanks.
> 
> I think this (and the earlier 5589e8) was caused by an unnoticed
> semantic conflict at 78089b71 ("Merge branch 'jc/name-rev-lw-tag'",
> 2017-05-30).  Merging is sometimes hard ;-)

Simple merges and semi-simple merges...

BTW, there's more fallout from those name-rev changes: In connection
with that other thread about surprising describe results for emacs.git I
noticed that I can easily get "git name-rev --stdin" to segfault there.
As easy as

echo bc5d96a0b2a1dccf7c459e40d21b54c977f4 | git name-rev --stdin

for example.

That's unfortunate for the use-case of name-rev to amend git log output.

The reason seems to be that with "--stdin" or "--all", "name-rev" walks
and names all commits before beginning to use that those names for even
a single commit as above.

That segfault bisects to the logic changing commit in
jc/name-rev-lw-tag, but I think the changed logic simply leads to more
xmallocs() the segfault sooner now. Or something that I dind't spot even
after a few hours.

On the other hand, nearly every time that I try to understand describe
or name-rev I want get rid of insert_commit_by_date() and the like and
replace this by generations, and maybe a simple rev-walk (per ref)...

> Will queue.
> 
>>  builtin/name-rev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> index c41ea7c2a6..598da6c8bc 100644
>> --- a/builtin/name-rev.c
>> +++ b/builtin/name-rev.c
>> @@ -253,7 +253,7 @@ static int name_ref(const char *path, const struct 
>> object_id *oid, int flags, vo
>>  struct commit *commit = (struct commit *)o;
>>  int from_tag = starts_with(path, "refs/tags/");
>>  
>> -if (taggerdate == ULONG_MAX)
>> +if (taggerdate == TIME_MAX)
>>  taggerdate = ((struct commit *)o)->date;
>>  path = name_ref_abbrev(path, can_abbreviate_output);
>>  name_rev(commit, xstrdup(path), taggerdate, 0, 0,

Re: signing commits using gpg2

[Michael J Gruber]

shawn wilson venit, vidit, dixit 02.09.2017 23:11:
> tl;dr - how do I get git to use gpg2 to sign things?
> 
> I'm using gpg2 (so no agent options are configured but an agent is
> running) which is configured w/ a Nitrokey (Pro if it matters):
> 
>  % git commit -m "Initial."
> 
>  gits/bash-libs (master ⚡) localhost
> gpg: detected reader `Nitrokey Nitrokey Pro (3467) 00 00'
> gpg: pcsc_connect failed: sharing violation (0x801b)
> gpg: apdu_send_simple(0) failed: locking failed
> Please insert the card and hit return or enter 'c' to cancel:
> gpg: pcsc_connect failed: sharing violation (0x801b)
> gpg: pcsc_connect failed: sharing violation (0x801b)
> gpg: apdu_send_simple(0) failed: locking failed
> Please insert the card and hit return or enter 'c' to cancel: c
> gpg: selecting openpgp failed: general error
> gpg: signing failed: general error
> gpg: signing failed: general error
> error: gpg failed to sign the data
> fatal: failed to write commit object
> 
> This works with gpg and ssh:

Not really...

>  % touch foo
> 
>  ~ localhost
>  % gpg2 --sign foo

... because you're using gpg2, not gpg.

> 
>  ~ localhost
> gpg: using "846FF490" as default secret key for signing
>  % cat foo*
> 
>  ~ localhost
> -BEGIN PGP MESSAGE-
> Version: GnuPG v2
> 
> owEBuQFG/pANAwAKAYwdY7SEb/SQAcsJYgNmb29ZqxfviQGcBAABCgAGBQJZqxfv
> AAoJEIwdY7SEb/SQAcEL/jonw+HymnlmfebtEwlvfx2Gl1Sbuw0xWWPpQ2Dtjljz
> HtpD+LWczjpOSMTHFNK9xPR2kcs1WNY+mO8M45QI7iDgFkKRzaxEqeNUJkoyF/+I
> 81VMmXDQMXFs4+8jy00b+UxTdvwdXaHMsOtu+6YCtmCR5Bzohg07ADsnXnGGn3Sd
> WTjVMzV6Dlh8LRF+coGJ8JuErBsRAI6vdNgJRVHYBULGNXci4uF/4a+58uiTL4/U
> PvC4ruXCNxCKi89nMERhwlnOvglseX3TDR5ldrc4Hzb+pLsj/l6N4sBW0Zmb8UcE
> 9BG3WjOs4eZvnLmk5XHrwisD2CXuHvyWMl0yH7LTrg+m4Itj0PJ4Px4H9E5t/zfs
> C1vcB/okcigeIyXnO06um02a5oZAYOKadB+6NRnBjULz5GvP2yxj/AO1VPmZprpt
> budMuHZcA0zNE3uBmcnQY5+1tdkyTrlTxsL58lQrn/U3wvgah3AXMEvjRGqbYWHj
> jDikQVJ7ESoevNqlfLPj8Q==
> =hV6v
> -END PGP MESSAGE-
> 
> However, if I try this w/ the old gpg:
> 
>  % gpg -ae -o foo.gpg foo
> 
>  ~ localhost
>  % gpg -d foo.gpg
> 
>  ~ localhost
> gpg: detected reader `Nitrokey Nitrokey Pro (3467) 00 00'
> gpg: pcsc_connect failed: sharing violation (0x801b)
> gpg: apdu_send_simple(0) failed: locking failed
> Please insert the card and hit return or enter 'c' to cancel: c
> gpg: selecting openpgp failed: general error
> gpg: encrypted with 3072-bit RSA key, ID 41826CFB, created 2017-03-13
>   "Shawn Wilson "
> gpg: public key decryption failed: general error
> gpg: decryption failed: secret key not available
>  % gpg2 -d foo.gpg
> 
>  ~ localhost
> gpg: encrypted with 3072-bit RSA key, ID E27FA0B841826CFB, created 2017-03-13
>   "Shawn Wilson "
> foo
> 
> (yeah I added data to the file)
> 
> And just to prove basic competency checking:
> 
>  % git config --global -l | grep sign
> 
>  ~ localhost
> user.signingkey=846FF490
> filter.gitconfig-rmuser.clean=sed -e "s/^\( *email =\).*/\1  address>/" -e "s/^\( *name =\).*/\1 /" -e "s/^\(
> *signingkey =\).*/\1 /"
> filter.gitconfig-rmuser.smudge=egrep "^ *(email|name|signingkey) = "
> commit.gpgsign=true
> 

So, gpg2 works and gpg does not. This is typical for the way in which
the gpg upgrade path is broken, and your distro installs gpg because it
still relies on it.

git sees two executables gpg and gpg2 and uses the first, so as to not
migrate your secrete key store inadvertently.

Short answer: Use

git config --global gpg.program gpg2

to make git use gpg2 which apparantly is your working gnupg setup.

Michael

[PATCH] name-rev: change ULONG_MAX to TIME_MAX

[Michael J Gruber]

Earlier, dddbad728c ("timestamp_t: a new data type for timestamps",
2017-04-26) changed several types to timestamp_t.

5589e87fd8 ("name-rev: change a "long" variable to timestamp_t",
2017-05-20) cleaned up a missed variable, but both missed a _MAX
constant.

Change the remaining constant to the one appropriate for the current
type

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/name-rev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index c41ea7c2a6..598da6c8bc 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -253,7 +253,7 @@ static int name_ref(const char *path, const struct 
object_id *oid, int flags, vo
struct commit *commit = (struct commit *)o;
int from_tag = starts_with(path, "refs/tags/");
 
-   if (taggerdate == ULONG_MAX)
+   if (taggerdate == TIME_MAX)
taggerdate = ((struct commit *)o)->date;
path = name_ref_abbrev(path, can_abbreviate_output);
name_rev(commit, xstrdup(path), taggerdate, 0, 0,
-- 
2.14.1.603.gf58147c36e

Re: git describe and "the smallest number of commits possible"

[Michael J Gruber]

Stefan Beller venit, vidit, dixit 28.08.2017 20:24:
> On Sat, Aug 26, 2017 at 7:47 AM, Kévin Le Gouguec
>  wrote:
>> Hi,
>>
>> I've asked this question on the git-users Google Groups list[1], and
>> while the answers there were interesting, I still cannot figure
>> whether my problem comes from an actual bug, a misleading manpage, my
>> inability to understand the code and/or the manpage, or a combination
>> of the three.
>>
>> I noticed this problem on 2.1.4 (Debian oldstable); I can reproduce it
>> on next (7ef03bb281b2220d0f2414365e4949beb2337042). Quoting
>> git-describe(1)'s SEARCH STRATEGY section:
>>
>>> If multiple tags were found during the walk then the tag which has
>>> the fewest commits different from the input commit-ish will be
>>> selected and output. Here fewest commits different is defined as the
>>> number of commits which would be shown by `git log tag..input` will
>>> be the smallest number of commits possible.
> 
> Maybe relevant
> https://public-inbox.org/git/20160421113004.ga3...@aepfle.de/
> (specifically the discussion after this patch, it sheds light on the
> heuristics used, though your case here doesn't use these heuristics)

git describe is driving my crazy sometimes, too - both the results and
the code ;)

[long read with lot's of debug output follows, sorry]

What is going on here is the following:
git describe --tags decides based on "depth".

git tag --merged dcc3ef3ee7 lists both emacs-25.1 and emac-25.2 (among
many others).

git rev-list --count emacs-25.1..dcc3ef3ee7
5126

git rev-list --count emacs-25.2..dcc3ef3ee7

4793

So, you would rightly expect 25.2 to be preferred over 25.1.

Now, "git describe" does not do a "rev-list --count" for each tag but,
instead, walks back from dcc3ef3ee7 and notes when it reaches a tagged
commit, and how many steps it takes to get there. The information
"reachable by the i-th tag that I encountered" is kept by a field of
width 27, which limits the --candidates values. Once the walk passes
more tags it is aborted.

Then the possible matches are sorted.

Then the depth calculation is finished.

Funny order of actions, isn't it? Smells like BUG.
At least it makes the debug output meaningless.

Here's the debug output before and after finish_depth_calculation():

searching to describe HEAD
 lightweight 4069 emacs-25.1
 lightweight 4086 emacs-25.1-rc2
 lightweight 4126 emacs-25.1-rc1
 annotated   4185 emacs-25.2
 annotated   4220 emacs-25.2-rc2
 lightweight 4226 emacs-25.0.95
 annotated   4236 emacs-25.2-rc1
 annotated   4280 emacs-25.1.91
 lightweight 4305 emacs-25.0.94
 lightweight 4329 emacs-25.1.90
traversed 4462 commits
more than 10 tags found; listed 10 most recent
gave up search at 5c587fdff164e8b90beb47f6da64b4884290e40a
[finish_depth_calculation()]
 lightweight   129847 emacs-25.1
 lightweight 4086 emacs-25.1-rc2
 lightweight 4126 emacs-25.1-rc1
 annotated   4185 emacs-25.2
 annotated   4220 emacs-25.2-rc2
 lightweight 4226 emacs-25.0.95
 annotated   4236 emacs-25.2-rc1
 annotated   4280 emacs-25.1.91
 lightweight 4305 emacs-25.0.94
 lightweight 4329 emacs-25.1.90
traversed 130257 commits
more than 10 tags found; listed 10 most recent
gave up search at 5c587fdff164e8b90beb47f6da64b4884290e40a
emacs-25.1-129847-gdcc3ef3ee7

Now, I can't claim that I can wrap my head around
finish_depth_calculation() and the way depth is calculated here, let
alone those huge numbers (probably per not-contained tag or so), but
that order of operations is clearly wrong.

Now, if we fix finish_depth_calculation() to actually do the work for
all possible candidates, and the sort afterwards, the result is:

LANG=C devgit describe --tags --debug --candidates=10
searching to describe HEAD
 lightweight   129847 emacs-25.1
 lightweight   129864 emacs-25.1-rc2
 lightweight   129904 emacs-25.1-rc1
 annotated 129981 emacs-25.2
 lightweight   130004 emacs-25.0.95
 annotated 130016 emacs-25.2-rc2
 annotated 130032 emacs-25.2-rc1
 annotated 130076 emacs-25.1.91
 lightweight   130083 emacs-25.0.94
 lightweight   130125 emacs-25.1.90
traversed 130257 commits
more than 10 tags found; listed 10 most recent
gave up search at 5c587fdff164e8b90beb47f6da64b4884290e40a
emacs-25.1-129847-gdcc3ef3ee7

LANG=C devgit describe --tags --debug --candidates=100

searching to describe HEAD
 annotated  13470 emacs-24.5-rc3-fixed
 lightweight13471 emacs-24.5-rc3
 lightweight13476 emacs-24.5-rc2
 lightweight13482 emacs-24.5-rc1
 lightweight13511 emacs-24.4.91
 lightweight13554 emacs-24.4.90
 lightweight13899 emacs-24.4
 lightweight13902 emacs-24.4-rc1
 lightweight13959 emacs-24.3.94
 annotated  13972 mh-e-8.6
 lightweight14049 emacs-24.3.93
 lightweight14176 emacs-24.3.92
 lightweight   129847 emacs-25.1
 lightweight   129864 emacs-25.1-rc2
 lightweight   129904 emacs-25.1-rc1
 lightweight   129971 emacs-25.0.92
 

Re: [PATCH v4 00/16] Fix git-gc losing objects in multi worktree

[Michael J Gruber]

Nguyễn Thái Ngọc Duy venit, vidit, dixit 23.08.2017 14:36:
> "git gc" when used in multiple worktrees ignore some per-worktree
> references: object references in the index, HEAD and reflog. This
> series fixes it by making the revision walker include these from all
> worktrees by default (and the series is basically split in three parts
> in the same order). There's a couple more cleanups in refs.c. Luckily
> it does not conflict with anything in 'pu'.
> 
> Compared to v3 [1], the largest change is supporting multi worktree in
> the reflog iterator. The merge iterator is now used (Micheal was right
> all along).
> 
> [1] https://public-inbox.org/git/20170419110145.5086-1-pclo...@gmail.com/
> 
> Nguyễn Thái Ngọc Duy (16):
>   revision.h: new flag in struct rev_info wrt. worktree-related refs
>   refs.c: use is_dir_sep() in resolve_gitlink_ref()
>   revision.c: refactor add_index_objects_to_pending()
>   revision.c: --indexed-objects add objects from all worktrees
>   refs.c: refactor get_submodule_ref_store(), share common free block
>   refs: move submodule slash stripping code to get_submodule_ref_store
>   refs: add refs_head_ref()
>   revision.c: use refs_for_each*() instead of for_each_*_submodule()
>   refs.c: move for_each_remote_ref_submodule() to submodule.c
>   refs: remove dead for_each_*_submodule()
>   revision.c: --all adds HEAD from all worktrees
>   files-backend: make reflog iterator go through per-worktree reflog
>   revision.c: --reflog add HEAD reflog from all worktrees
>   rev-list: expose and document --single-worktree
>   refs.c: remove fallback-to-main-store code get_submodule_ref_store()
>   refs.c: reindent get_submodule_ref_store()

I probably won't be able to review this (many commits without commit
message), but I'm happy to see progress on the "--all" and prune front
(and will test). I suggest we think about the UI exposure a bit when it
comes to including all heads or naming options, though:

* HEAD is "the current head"
* refs/heads is where all local branch heads are

* --branches is the rev-list/log option for refs/heads/*
* --all is the rev-list/log option for refs/* plus HEAD
* HEAD is the rev-list/log argument for HEAD

* --heads is the show-ref option limiting to refs/heads/*
* --head is the show-ref option which adds HEAD

* refs/heads is the for-each-ref-pattern for refs/heads/*
* HEAD is not the for-each-ref-pattern for HEAD
[I'll suggest a patch to change the latter, shortly.]

I would hope that the result of this series and other efforts will be:

* consistent way to specify "all local branch heads"
* consistent way to specify "the head" aka HEAD
* consistent way to specify "all linked worktree heads"
[* maybe something for submodules...]

This may require changing the misnamed show-ref option, but also
thinking twice before changing the meaning of "--all" for the
rev-list/log family: it's easy to say "--all --linked" or "--all
--heads" to get everything plus all linked worktree heads when "--all"
== "--branches --tags HEAD", but it's more cumbersome with a changed
--all that is "really everything". I gues my suggestion would be:

--all as it is now (refs/* plus HEAD)

--head alternative way to say HEAD (as it is now for show-ref)

--heads HEAD for all linked worktrees (incompatible change for show-ref)

And all of them should work the same for the rev-list/log family as well
as for-each-ref/show-ref.

I thinking that changing show-ref (porcelain, not quite as commonly
used) should do the least harm compared to all other options.

Michael

[PATCH v3 0/4] Keep merge during kills

[Michael J Gruber]

Compared to the 3-item v2:

1/4 == 1/3

2/4 is new as per Junio's suggestion: clarify the call-chain leading up
to prepare_to_commit()

3/4 == 2/3 with amended subject

4/4 is 3/3 rebased, squash-if removed

Michael J Gruber (4):
  Documentation/git-merge: explain --continue
  merge: clarify call chain
  merge: split write_merge_state in two
  merge: save merge state earlier

 Documentation/git-merge.txt |  5 -
 builtin/merge.c | 15 ---
 t/t7600-merge.sh| 15 +++
 3 files changed, 31 insertions(+), 4 deletions(-)

-- 
2.14.1.426.g4352aa77a5

[PATCH v3 4/4] merge: save merge state earlier

[Michael J Gruber]

If the `git merge` process is killed while waiting for the editor to
finish, the merge state is lost but the prepared merge msg and tree is kept.
So, a subsequent `git commit` creates a squashed merge even when the
user asked for proper merge commit originally.

Demonstrate the problem with a test crafted after the in t7502. The test
requires EXECKEEPSPID (thus does not run under MINGW).

Save the merge state earlier (in the non-squash case) so that it does
not get lost. This makes the test pass.

Reported-by: hIpPy <hippy2...@gmail.com>
Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/merge.c  |  2 ++
 t/t7600-merge.sh | 15 +++
 2 files changed, 17 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index db3335b3bf..2d5c45a904 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -758,6 +758,7 @@ N_("Please enter a commit message to explain why this merge 
is necessary,\n"
"Lines starting with '%c' will be ignored, and an empty message aborts\n"
"the commit.\n");
 
+static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
struct strbuf msg = STRBUF_INIT;
@@ -769,6 +770,7 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
strbuf_commented_addf(, _(merge_editor_comment), 
comment_line_char);
if (signoff)
append_signoff(, ignore_non_trailer(msg.buf, msg.len), 0);
+   write_merge_heads(remoteheads);
write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
if (run_commit_hook(0 < option_edit, get_index_file(), 
"prepare-commit-msg",
git_path_merge_msg(), "merge", NULL))
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 2ebda509ac..80194b79f9 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -774,4 +774,19 @@ test_expect_success 'merge can be completed with 
--continue' '
verify_parents $c0 $c1
 '
 
+write_script .git/FAKE_EDITOR <> .git/FAKE_EDITOR
+ GIT_EDITOR=.git/FAKE_EDITOR
+ export GIT_EDITOR
+ exec git merge --no-ff --edit c1'\'' &&
+   git merge --continue &&
+   verify_parents $c0 $c1
+'
+
 test_done
-- 
2.14.1.426.g4352aa77a5

[PATCH v3 2/4] merge: clarify call chain

[Michael J Gruber]

prepare_to_commit() cannot be reached in the non-squash case:
It is called by merge_trivial() and finish_automerge() only, but the
calls to the latter are somewhat hard to track:

If option_commit is not set, the code in cmd_merge() uses a fake
conflict return code (ret=1) to avoid writing the tree, which also
avoids setting automerge_was_ok (just as in the proper ret==1 case), so
that finish_automerge() is not called.

To ensure that no code change breaks that assumption, safe-guard
prepare_to_commit() by a BUG() statement.

Suggested-by: junio
Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/merge.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index cc57052993..dafec80fa9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -763,6 +763,8 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
struct strbuf msg = STRBUF_INIT;
strbuf_addbuf(, _msg);
strbuf_addch(, '\n');
+   if (squash)
+   BUG("the control must not reach here under --squash");
if (0 < option_edit)
strbuf_commented_addf(, _(merge_editor_comment), 
comment_line_char);
if (signoff)
-- 
2.14.1.426.g4352aa77a5

[PATCH v3 3/4] merge: split write_merge_state in two

[Michael J Gruber]

write_merge_state() writes out the merge heads, mode, and msg. But we
may want to write out heads, mode without the msg. So, split out heads
(+mode) into a separate function write_merge_heads() that is called by
write_merge_state().

No funtional change so far, except when these non-atomic writes are
interrupted: we write heads-mode-msg now when we used to write
heads-msg-mode.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/merge.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index dafec80fa9..db3335b3bf 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -910,7 +910,7 @@ static int setup_with_upstream(const char ***argv)
return i;
 }
 
-static void write_merge_state(struct commit_list *remoteheads)
+static void write_merge_heads(struct commit_list *remoteheads)
 {
struct commit_list *j;
struct strbuf buf = STRBUF_INIT;
@@ -926,8 +926,6 @@ static void write_merge_state(struct commit_list 
*remoteheads)
strbuf_addf(, "%s\n", oid_to_hex(oid));
}
write_file_buf(git_path_merge_head(), buf.buf, buf.len);
-   strbuf_addch(_msg, '\n');
-   write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
 
strbuf_reset();
if (fast_forward == FF_NO)
@@ -935,6 +933,13 @@ static void write_merge_state(struct commit_list 
*remoteheads)
write_file_buf(git_path_merge_mode(), buf.buf, buf.len);
 }
 
+static void write_merge_state(struct commit_list *remoteheads)
+{
+   write_merge_heads(remoteheads);
+   strbuf_addch(_msg, '\n');
+   write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
+}
+
 static int default_edit_option(void)
 {
static const char name[] = "GIT_MERGE_AUTOEDIT";
-- 
2.14.1.426.g4352aa77a5

[PATCH v3 1/4] Documentation/git-merge: explain --continue

[Michael J Gruber]

Currently, 'git merge --continue' is mentioned but not explained.

Explain it.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 Documentation/git-merge.txt | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 6b308ab6d0..615e6bacde 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -288,7 +288,10 @@ After seeing a conflict, you can do two things:
 
  * Resolve the conflicts.  Git will mark the conflicts in
the working tree.  Edit the files into shape and
-   'git add' them to the index.  Use 'git commit' to seal the deal.
+   'git add' them to the index.  Use 'git commit' or
+   'git merge --continue' to seal the deal. The latter command
+   checks whether there is a (interrupted) merge in progress
+   before calling 'git commit'.
 
 You can work through the conflict with a number of tools:
 
-- 
2.14.1.426.g4352aa77a5

Re: [PATCH v2 3/3] merge: save merge state earlier

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 22.08.2017 02:38:
> Michael J Gruber <g...@grubix.eu> writes:
> 
>>  static void prepare_to_commit(struct commit_list *remoteheads)
>>  {
>>  struct strbuf msg = STRBUF_INIT;
>> @@ -767,6 +768,8 @@ static void prepare_to_commit(struct commit_list 
>> *remoteheads)
>>  strbuf_commented_addf(, _(merge_editor_comment), 
>> comment_line_char);
>>  if (signoff)
>>  append_signoff(, ignore_non_trailer(msg.buf, msg.len), 0);
>> +if (!squash)
>> +write_merge_heads(remoteheads);
>>  write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
>>  if (run_commit_hook(0 < option_edit, get_index_file(), 
>> "prepare-commit-msg",
>>  git_path_merge_msg(), "merge", NULL))
> 
> I can understand that you would never want to write out MERGE_HEAD
> while squashing, but I somehow think it would be a bug in the caller
> to call prepare_to_commit(), whose point is to prepare the merge
> message to be recorded in the resulting merge commit, when the user
> gave us the "--squash" option, which is an explicit instruction that
> the user does not want the merge commit the message is used.

That sounds reasonable. I vaguely remember a failing test for an earlier
version that I tried, but that was before the "split".

> Can squash ever be true in this function?
> 
> This function has two callsites: merge_trivial() and
> finish_automerge().
> 
> I think merge_trivial() will not be called under "--squash", which
> turns option_commit off and the only callsite of it is inside an
> else-if clause that requres option_commit to be true.  You can do a
> similar deduction around the "automerge_was_ok" variable to see if
> finish_automerge() can be called when "--squash" is given; I suspect
> the answer may be no.

I'll go without the if, after more testing.

>> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
>> index 2ebda509ac..80194b79f9 100755
>> --- a/t/t7600-merge.sh
>> +++ b/t/t7600-merge.sh
>> @@ -774,4 +774,19 @@ test_expect_success 'merge can be completed with 
>> --continue' '
>>  verify_parents $c0 $c1
>>  '
>>  
>> +write_script .git/FAKE_EDITOR <> +# kill -TERM command added below.
>> +EOF
>> +
>> +test_expect_success EXECKEEPSPID 'killed merge can be completed with 
>> --continue' '
>> +git reset --hard c0 &&
>> +! "$SHELL_PATH" -c '\''
>> +  echo kill -TERM $$ >> .git/FAKE_EDITOR
>> +  GIT_EDITOR=.git/FAKE_EDITOR
>> +  export GIT_EDITOR
>> +  exec git merge --no-ff --edit c1'\'' &&
> 
> This is a tricky construct.  You "reserve" a process ID by using a
> shell, arrange it to be killed and then using "exec" to make it the
> "git merge" program to be killed.  I kind of like the convolutedness.

That is from t7502. Sorry for hiding that note in the cover letter, I
should put it into 3/3's message or a test comment.

When testing, I simply used "git merge... &" and "ps" or "jobs" to know
which process to kill. Apparantly, the above is the most portable way to
script that. t7502 went through a few iterations to ensure this.

>> +git merge --continue &&
>> +verify_parents $c0 $c1
>> +'
>> +
>>  test_done

Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue

[Michael J Gruber]

Martin Ågren venit, vidit, dixit 21.08.2017 18:43:
> On 21 August 2017 at 14:53, Michael J Gruber <g...@grubix.eu> wrote:
>> Currently, 'git merge --continue' is mentioned but not explained.
>>
>> Explain it.
>>
>> Signed-off-by: Michael J Gruber <g...@grubix.eu>
>> ---
>>  Documentation/git-merge.txt | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
>> index 6b308ab6d0..615e6bacde 100644
>> --- a/Documentation/git-merge.txt
>> +++ b/Documentation/git-merge.txt
>> @@ -288,7 +288,10 @@ After seeing a conflict, you can do two things:
>>
>>   * Resolve the conflicts.  Git will mark the conflicts in
>> the working tree.  Edit the files into shape and
>> -   'git add' them to the index.  Use 'git commit' to seal the deal.
>> +   'git add' them to the index.  Use 'git commit' or
>> +   'git merge --continue' to seal the deal. The latter command
>> +   checks whether there is a (interrupted) merge in progress
>> +   before calling 'git commit'.
>>
>>  You can work through the conflict with a number of tools:
> 
> There are actually two things going on here. First, this mentions git
> merge --continue. Second, it explains what that command does. But the
> latter is done earlier (not exactly like here, but still).

I didn't see that explained in the man page at all - on the contrary, I
only saw a forward reference (see section...), but then only an
explanation of what "resolving" means (including the "git commit"-step).
It is unclear to me from the man page which steps of "resolving" the
command "git merge --continue" does - you could think it does "git
commit -a", for example.

> When git merge --continue originally appeared, this part of the docs was
> discussed briefly. Maybe interesting:
> 
> https://public-inbox.org/git/xmqq60mn671x@gitster.mtv.corp.google.com/
> 
> Martin
> 

[PATCH v2 2/3] merge: split write_merge_state in two

[Michael J Gruber]

write_merge_state() writes out the merge heads, mode, and msg. But we
may want to write out heads, mode without the msg. So, split out heads
(+mode) into a separate function write_merge_heads() that is called by
write_merge_state().

No funtional change so far.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/merge.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index cc57052993..86f0adde3b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -908,7 +908,7 @@ static int setup_with_upstream(const char ***argv)
return i;
 }
 
-static void write_merge_state(struct commit_list *remoteheads)
+static void write_merge_heads(struct commit_list *remoteheads)
 {
struct commit_list *j;
struct strbuf buf = STRBUF_INIT;
@@ -924,8 +924,6 @@ static void write_merge_state(struct commit_list 
*remoteheads)
strbuf_addf(, "%s\n", oid_to_hex(oid));
}
write_file_buf(git_path_merge_head(), buf.buf, buf.len);
-   strbuf_addch(_msg, '\n');
-   write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
 
strbuf_reset();
if (fast_forward == FF_NO)
@@ -933,6 +931,13 @@ static void write_merge_state(struct commit_list 
*remoteheads)
write_file_buf(git_path_merge_mode(), buf.buf, buf.len);
 }
 
+static void write_merge_state(struct commit_list *remoteheads)
+{
+   write_merge_heads(remoteheads);
+   strbuf_addch(_msg, '\n');
+   write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
+}
+
 static int default_edit_option(void)
 {
static const char name[] = "GIT_MERGE_AUTOEDIT";
-- 
2.14.1.364.ge466dba5f7

[PATCH v2 0/3] Keep merge during kills

[Michael J Gruber]

So here's a little series.

1/3 I just noted along the way

2/3 splits the merge state writing into parts as a preparatory step.
This is something we may want to carry further and get rid of some other
places which write merge_msg unneccessarily so that merge performance
does not suffer because of the additional write.

3/3 comes with a test now. It's crafted after t7502 and does not work
under MINGW, unfortunately. But the fix does :)

I'm still asking for comments whether this has other side effects. I've
been told there are people who do a lot of merges regularly, and they're
not the kind of people that I'd like to make angry - at least not
without a good reason.

Michael J Gruber (3):
  Documentation/git-merge: explain --continue
  merge: split write_merge_state in two
  merge: save merge state earlier

 Documentation/git-merge.txt |  5 -
 builtin/merge.c | 14 +++---
 t/t7600-merge.sh| 15 +++
 3 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.14.1.364.ge466dba5f7

[PATCH v2 3/3] merge: save merge state earlier

[Michael J Gruber]

If the `git merge` process is killed while waiting for the editor to
finish, the merge state is lost but the prepared merge msg and tree is kept.
So, a subsequent `git commit` creates a squashed merge even when the
user asked for proper merge commit originally.

Save the merge state earlier (in the non-squash case) so that it does
not get lost.

Reported-by: hIpPy <hippy2...@gmail.com>
Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/merge.c  |  3 +++
 t/t7600-merge.sh | 15 +++
 2 files changed, 18 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 86f0adde3b..5379b08824 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -758,6 +758,7 @@ N_("Please enter a commit message to explain why this merge 
is necessary,\n"
"Lines starting with '%c' will be ignored, and an empty message aborts\n"
"the commit.\n");
 
+static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
struct strbuf msg = STRBUF_INIT;
@@ -767,6 +768,8 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
strbuf_commented_addf(, _(merge_editor_comment), 
comment_line_char);
if (signoff)
append_signoff(, ignore_non_trailer(msg.buf, msg.len), 0);
+   if (!squash)
+   write_merge_heads(remoteheads);
write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
if (run_commit_hook(0 < option_edit, get_index_file(), 
"prepare-commit-msg",
git_path_merge_msg(), "merge", NULL))
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 2ebda509ac..80194b79f9 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -774,4 +774,19 @@ test_expect_success 'merge can be completed with 
--continue' '
verify_parents $c0 $c1
 '
 
+write_script .git/FAKE_EDITOR <> .git/FAKE_EDITOR
+ GIT_EDITOR=.git/FAKE_EDITOR
+ export GIT_EDITOR
+ exec git merge --no-ff --edit c1'\'' &&
+   git merge --continue &&
+   verify_parents $c0 $c1
+'
+
 test_done
-- 
2.14.1.364.ge466dba5f7

[PATCH v2 1/3] Documentation/git-merge: explain --continue

[Michael J Gruber]

Currently, 'git merge --continue' is mentioned but not explained.

Explain it.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 Documentation/git-merge.txt | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 6b308ab6d0..615e6bacde 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -288,7 +288,10 @@ After seeing a conflict, you can do two things:
 
  * Resolve the conflicts.  Git will mark the conflicts in
the working tree.  Edit the files into shape and
-   'git add' them to the index.  Use 'git commit' to seal the deal.
+   'git add' them to the index.  Use 'git commit' or
+   'git merge --continue' to seal the deal. The latter command
+   checks whether there is a (interrupted) merge in progress
+   before calling 'git commit'.
 
 You can work through the conflict with a number of tools:
 
-- 
2.14.1.364.ge466dba5f7

[PATCH] merge: save merge state earlier

[Michael J Gruber]

If the `git merge` process is killed while waiting for the editor to
finish, the merge state is lost but the prepared merge msg and tree is kept.
So, a subsequent `git commit` creates a squashed merge even when the
user asked for proper merge commit originally.

Save the merge state earlier (in the non-squash case) so that it does
not get lost.

Reported-by: hIpPy <hippy2...@gmail.com>
Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/merge.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index cc57052993..5379b08824 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -758,6 +758,7 @@ N_("Please enter a commit message to explain why this merge 
is necessary,\n"
"Lines starting with '%c' will be ignored, and an empty message aborts\n"
"the commit.\n");
 
+static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
struct strbuf msg = STRBUF_INIT;
@@ -767,6 +768,8 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
strbuf_commented_addf(, _(merge_editor_comment), 
comment_line_char);
if (signoff)
append_signoff(, ignore_non_trailer(msg.buf, msg.len), 0);
+   if (!squash)
+   write_merge_heads(remoteheads);
write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
if (run_commit_hook(0 < option_edit, get_index_file(), 
"prepare-commit-msg",
git_path_merge_msg(), "merge", NULL))
@@ -908,7 +911,7 @@ static int setup_with_upstream(const char ***argv)
return i;
 }
 
-static void write_merge_state(struct commit_list *remoteheads)
+static void write_merge_heads(struct commit_list *remoteheads)
 {
struct commit_list *j;
struct strbuf buf = STRBUF_INIT;
@@ -924,8 +927,6 @@ static void write_merge_state(struct commit_list 
*remoteheads)
strbuf_addf(, "%s\n", oid_to_hex(oid));
}
write_file_buf(git_path_merge_head(), buf.buf, buf.len);
-   strbuf_addch(_msg, '\n');
-   write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
 
strbuf_reset();
if (fast_forward == FF_NO)
@@ -933,6 +934,13 @@ static void write_merge_state(struct commit_list 
*remoteheads)
write_file_buf(git_path_merge_mode(), buf.buf, buf.len);
 }
 
+static void write_merge_state(struct commit_list *remoteheads)
+{
+   write_merge_heads(remoteheads);
+   strbuf_addch(_msg, '\n');
+   write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
+}
+
 static int default_edit_option(void)
 {
static const char name[] = "GIT_MERGE_AUTOEDIT";
-- 
2.14.1.364.ge466dba5f7

Re: Git makes a merge commit but as a normal (non-merge) commit

[Michael J Gruber]

hIpPy venit, vidit, dixit 19.08.2017 00:35:
> While merging if I do certain actions then the merge commit is made
> with the merge message but as a normal (non-merge) commit.
> 
> Repro steps:
> - Set GIT_MERGE_AUTOEDIT=yes (set other than "no") in .bashrc
> - Make a merge commit with no conflicts.
>   (external text editor shows the generated merge message)
> - Focus on Git Bash and Ctrl-C.
> - Commit (git commit).
> 
> Actual behavior:
> Git makes a normal (non-merge) commit (squash merge) but with the
> merge commit message.
> 
> It looks like a bug to me. This is very confusing later on as the repo
> topology would show that the branch is not merged in and there is not
> an easy way to find out when the merge was made.
> 
> Expected behavior:
> Git should stay in a MERGING state. The user can choose to either
> abort the merge or continue the merge (git merge --continue OR git
> commit).
> 
> This does not happen in case of conflicts (at least I'm not able to
> repro). I get a (master|MERGING) prompt till I resolve the conflicts
> and commit, which goes through correctly as a merge commit.
> 
> Environment:
> $ git version
> git version 2.14.0.windows.2
> $ bash --version
> GNU bash, version 4.4.12(1)-release (x86_64-pc-msys)

Thanks for the detailed info!

It turns out that his is not Windows specific. The recipe is:
- make "git merge" call and wait for an editor
- kill "git merge" while it waits for the editor

The effect is that prepare_to_commit() has no chance to call
abort_commit() any more (which would call write_merge_state())..

Now, I'm not sure why we only do half of write_merge_state() (write out
the merge_msg, but not merge_head) before launching the editor.

In any case, the following patch solves the problem and passes the
existing tests. But I'll wait for more comments (and think about a test
meanwhile).

Michael

[PATCH 2/2] Documentation/git-for-each-ref: clarify peeling of tags for --format

[Michael J Gruber]

`*` in format strings means peeling of tag objects so that object field
names refer to the object that the tag object points at, instead of the
tag object itself.

Currently, this is documented using grammar that is clearly inspired by
classical latin, though missing more than an article in order to be
classical english.

Try and straighten that explanation out a bit.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 Documentation/git-for-each-ref.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index dac9138fab..bb370c9c7b 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -41,8 +41,9 @@ OPTIONS
A string that interpolates `%(fieldname)` from a ref being shown
and the object it points at.  If `fieldname`
is prefixed with an asterisk (`*`) and the ref points
-   at a tag object, the value for the field in the object
-   tag refers is used.  When unspecified, defaults to
+   at a tag object, use the value for the field in the object
+   which the tag object refers to (instead of the field in the tag object).
+   When unspecified, `` defaults to
`%(objectname) SPC %(objecttype) TAB %(refname)`.
It also interpolates `%%` to `%`, and `%xx` where `xx`
are hex digits interpolates to character with hex code
-- 
2.14.1.224.g15ee91971c

[PATCH 1/2] Documentation: use proper wording for ref format strings

[Michael J Gruber]

Various commands list refs and allow to use a format string for the
output that interpolates from the ref as well as the object it points
at (for-each-ref; branch and tag in list mode).

Currently, the documentation talks about interpolating from the object.
This is confusing because a ref points to an object but not vice versa,
so the object cannot possible know %(refname), for example. Thus, this is
wrong independent of refs being objects (one day, maybe) or not.

Change the wording to make this clearer (and distinguish it from formats
for the log family).

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 Documentation/git-branch.txt   | 4 ++--
 Documentation/git-for-each-ref.txt | 4 ++--
 Documentation/git-tag.txt  | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 81bd0a7b77..d0b3358771 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -267,8 +267,8 @@ start-point is either a local or remote-tracking branch.
Only list branches of the given object.
 
 --format ::
-   A string that interpolates `%(fieldname)` from the object
-   pointed at by a ref being shown.  The format is the same as
+   A string that interpolates `%(fieldname)` from a branch ref being shown
+   and the object it points at.  The format is the same as
that of linkgit:git-for-each-ref[1].
 
 Examples
diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index cc42c12832..dac9138fab 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -38,8 +38,8 @@ OPTIONS
key.
 
 ::
-   A string that interpolates `%(fieldname)` from the
-   object pointed at by a ref being shown.  If `fieldname`
+   A string that interpolates `%(fieldname)` from a ref being shown
+   and the object it points at.  If `fieldname`
is prefixed with an asterisk (`*`) and the ref points
at a tag object, the value for the field in the object
tag refers is used.  When unspecified, defaults to
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index d97aad3439..543fb425ee 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -188,8 +188,8 @@ This option is only applicable when listing tags without 
annotation lines.
Defaults to HEAD.
 
 ::
-   A string that interpolates `%(fieldname)` from the object
-   pointed at by a ref being shown.  The format is the same as
+   A string that interpolates `%(fieldname)` from a tag ref being shown
+   and the object it points at.  The format is the same as
that of linkgit:git-for-each-ref[1].  When unspecified,
defaults to `%(refname:strip=2)`.
 
-- 
2.14.1.224.g15ee91971c

Re: Weirdness with git change detection

[Michael J Gruber]

Jeff King venit, vidit, dixit 11.07.2017 10:24:
> On Tue, Jul 11, 2017 at 10:20:43AM +0200, Torsten Bögershausen wrote:
> 
>>> No problem. I actually think it would be interesting if Git could
>>> somehow detect and warn about this situation. But the obvious way to do
>>> that would be to re-run the clean filter directly after checkout. And
>>> doing that all the time is expensive.
>>
>> Would it be possible to limit the round-trip-check to "git reset --hard" ?
>> If yes, possibly many users are willing to pay the price, if Git tells
>> the "your filters don't round-trip". (Including CRLF conversions)
> 
> Anything's possible, I suppose. But I don't think I'd want that feature
> turned on myself.
> 
>>> Perhaps some kind of "lint" program would be interesting to warn of
>>> possible misconfigurations. Of course people would have to run it for it
>>> to be useful. :)
>>
>> What do you have in mind here ?
>> Don't we need to run some content through the filter(s)?
> 
> I was thinking of a tool that could run a series of checks on the
> repository and nag about potential problems. One of them could be doing
> a round-trip repo->clean->smudge for each file.
> 
> Another one might be warning about files that differ only in case.
> 
> The idea being that users could run "git lint" if they suspect something
> funny is going on. I dunno. It may be a dead-end. Most such
> oddities are better detected and handled during actual git operations if
> we can. So this would really just be for things that are too expensive
> to detect in normal operations.
> 
> -Peff
> 

Typically, that problem arises when you turn a filter on or off at some
point in your history. Since "attributes" can come from various sources,
especially the versioned ".gitattributes" file, unversioned per-repo
.git/info/attributes, and global attributes, "git diff" may apply
different attributes depending on what you diff (versioned blob, workdir
file, out-of-tree file).

This is not made easier by the fact that unversioned config (per repo,
per user, global) defines the filter action, and that even upgrades of
your filter tools may change the output. So, "filter off/on" is by no
means the only possible source of discrepancies.

I've found that when I decide to use a filter like that, the best
approach is to either apply it retroactively (filter-branch,
unversionsed attributes, that is clean all stored blobs) or make a
commit where I specifically note the switch (versioned .gitattributes
plus affected blob changes) and what config should go along with it.

All of this is difficult to check or correct automatically, since it
depends on user decisions.

About the only thing we could do is checking that
"clean(smudge(foo))=clean(foo)" at a specific "point in time"
(attributes, config) for specific foo, but that wouldn't catch the case
above, even if we iterated over all commits which affect files that the
filter (currently) applies to.

Keep in mind that filters are a killer feature, so if you shoot yourself
in the foot: it could have come worse ;)

Michael

Re: [PATCH] t5534: fix misleading grep invocation

[Michael J Gruber]

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.
>>
>> Fix the test by avoiding the newlines in the patterns.
>>
>> Signed-off-by: Johannes Schindelin 
>> ---
> 
> The invocation this fixes is not just misleading but simply wrong.
> Nicely spotted.

In addition, the patch makes sure to catch any rev-parse failures which
the original invocation shove under the rug.

> Thanks, will queue.

Thanks from the faithful copy-editor ;)

How did you spot this? Are there grep versions that behave differently?

>>  t/t5534-push-signed.sh | 14 ++
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
>> index 5bcb288f5c4..464ffdd147a 100755
>> --- a/t/t5534-push-signed.sh
>> +++ b/t/t5534-push-signed.sh
>> @@ -119,8 +119,11 @@ test_expect_success GPG 'signed push sends push 
>> certificate' '
>>  sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
>>  ) >expect &&
>>  
>> -grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
>> -grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
>> +noop=$(git rev-parse noop) &&
>> +ff=$(git rev-parse ff) &&
>> +noff=$(git rev-parse noff) &&
>> +grep "$noop $ff refs/heads/ff" dst/push-cert &&
>> +grep "$noop $noff refs/heads/noff" dst/push-cert &&
>>  test_cmp expect dst/push-cert-status
>>  '
>>  
>> @@ -200,8 +203,11 @@ test_expect_success GPG 'fail without key and heed 
>> user.signingkey' '
>>  sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
>>  ) >expect &&
>>  
>> -grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
>> -grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
>> +noop=$(git rev-parse noop) &&
>> +ff=$(git rev-parse ff) &&
>> +noff=$(git rev-parse noff) &&
>> +grep "$noop $ff refs/heads/ff" dst/push-cert &&
>> +grep "$noop $noff refs/heads/noff" dst/push-cert &&
>>  test_cmp expect dst/push-cert-status
>>  '
>>  
>>
>> base-commit: 5116f791c12dda6b6c22fa85b600a8e30dfa168a

Re: [PATCH] tests: remove the GETTEXT_POISON compile-time option to test i18n marking

[Michael J Gruber]

[Turns out I still can't operate gmail's web interface. Sorry for the dupe.]

2017-04-24 13:04 GMT+02:00 Ævar Arnfjörð Bjarmason :
> Remove the GETTEXT_POISON=YesPlease compile-time which turns all of
> git's LC_*=C output into strings like "# GETTEXT POISON #" instead of
> gettext(msgid).
>
> See commit bb946bba76 ("i18n: add GETTEXT_POISON to simulate
> unfriendly translator", 2011-02-22) for what this was originally
> intended for.
>
> This facility has been broken for quite a while and has been subjected
> to frequent bitrot. The initial idea behind it back when it was added
> in 2011 was to prevent the accidental translation of plumbing
> messages.
>
> This isn't a big concern anymore as git isn't mass-adding i18n
> messages for a newly developed i18n facility as it was back then,
> maintaining this facility incurs a burden, and in actuality this has
> often been broken long enough for potential plumbing messages to be
> translated & make their way into major releases anyway.
>
> Most of this patch consists of search/replacing the test suite for:
>
> test_i18ngrep ! -> ! grep
> test_i18ngrep   -> grep
> test_i18ncmp-> test_cmp
>
> 1. 

Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease

[Michael J Gruber]

Ævar Arnfjörð Bjarmason venit, vidit, dixit 20.04.2017 23:58:
> As a refresh of everyone's memory (because mine needed it). This is a
> feature I added back in 2011 when the i18n support was initially
> added.
> 
> There was concern at the time that we would inadvertently mark
> plumbing messages for translation, particularly something in a shared
> code path, and this was a way to hopefully smoke out those issues with
> the test suite.
> 
> However compiling with it breaks a couple of dozen tests, I stopped
> digging when I saw some broke back in 2014.
> 
> What should be done about this? I think if we're going to keep them
> they need to be run regularly by something like Travis (Lars CC'd),
> however empirical evidence suggests that not running them is just fine
> too, so should we just remove support for this test mode?
> 
> I don't care, but I can come up with the patch either way, but would
> only be motivated to write the one-time fix for it if some CI system
> is actually running them regularly, otherwise they'll just be subject
> to bitrotting again.
> 

I use that switch when I change something that involves l10n, but
usually I run specific tests only. To be honest: I have to make sure not
to get confused by (nor forget one of) the build flag GETTEXT_POISON and
the environment variable GIT_GETTEXT_POISON. I'm not sure I always
tested what I meant to test...

Michael

Re: Draft of Git Rev News edition 26

[Michael J Gruber]

Christian Couder venit, vidit, dixit 17.04.2017 16:33:
> Hi,
> 
> A draft of a new Git Rev News edition is available here:
> 
>   
> https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-26.md
> 
> Everyone is welcome to contribute in any section either by editing the
> above page on GitHub and sending a pull request, or by commenting on
> this GitHub issue:
> 
>   https://github.com/git/git.github.io/issues/238
> 
> You can also reply to this email.
> 
> In general all kinds of contribution, for example proofreading,
> suggestions for articles or links, help on the issues in GitHub, and
> so on, are very much appreciated.
> 
> I tried to cc everyone who appears in this edition, but maybe I missed
> some people, sorry about that.
> 
> Thomas, Jakub, Markus and myself plan to publish this edition on
> Wednesday April 19th.
> 
> Thanks,
> Christian.

Hi Christian,

Thanks for the ping on the draft.

Re gpg: Maybe some valuable point of information is what Werner Koch
himself said in that thread:
"That [the command line is not a stable API to GnuPG] is not true.  The
command line interface has been stable for the
last 19 years.  We only removed a left over PGP-2 backward compatibility
in 2.1 (-kvv).  I doubt that this has even been noticed."

I think our conclusion was that on Git's side, there is no problem to
solve (except, maybe, to use gpg2 by default when gpg is not installed)
because the main problem is mixed installations of gpg1 and gpg2.1+, and
we don't want to use a library instead of the command line API for the
reasons mentioned by Linus and others.

Michael

Re: `git status` output is very misleading after a merge on a "detached HEAD"

[Michael J Gruber]

Ævar Arnfjörð Bjarmason venit, vidit, dixit 12.04.2017 14:18:
> On Wed, Apr 12, 2017 at 7:43 AM, Michael J Gruber <g...@grubix.eu> wrote:
>> Am 11. April 2017 22:40:14 MESZ schrieb "Ævar Arnfjörð Bjarmason" 
>> <ava...@gmail.com>:
>>> On Tue, Apr 11, 2017 at 5:13 PM, Enis Bayramoğlu
>>>> HEAD detached from origin/master 1 commit ago,
>>>
>>> In lieu of that, which would need some of the rev-list machinery to be
>>> invoked on every git-status, I wonder if just saying "HEAD detached &
>>> diverged from origin/master" wouldn't be clearer:
>>>
>>> diff --git a/wt-status.c b/wt-status.c
>>> index 308cf3779e..79c8cfd1cf 100644
>>> --- a/wt-status.c
>>> +++ b/wt-status.c
>>> @@ -1542,7 +1542,7 @@ static void wt_longstatus_print(struct wt_status
>>> *s)
>>>if (state.detached_at)
>>>  on_what = _("HEAD detached at ");
>>>else
>>> -   on_what = _("HEAD detached from
>>> ");
>>> +   on_what = _("HEAD detached &
>>> diverged from ");
>>>} else {
>>>branch_name = "";
>>>   on_what = _("Not currently on any branch.");
>>>
>>>
>>>
>>
>> No way. That would reduce the information that we give.
> 
> How does it reduce the information we give? Maybe I've missed
> something about what "from" means here, as opposed to "at", but it
> seems to me to mean the same thing as "diverged" by definition, i.e.
> we detached from the branch but we diverged from it.

No, "at" means we're still at that commit - detached but not diverged.
"from" means we only started from that commit, but are not at it any more.

> Saying "diverged"
> just makes it clearer, how does it reduce the information we give?

I misread your patch on my mobile phone, sorry. I thought you meant to
replace both "at" and "from" by "diverged from" because you considered
them synonymous.

But your patch touches just the" from" case and emphasizes the "diverge"
aspect, which is fine, of course.

>> Note that the difference between from and at is also: are there commits that 
>> we could lose when we switch away, that is: that git checkout would warn us 
>> about?
> 
> Right, but I don't see how that's in any way conflicting or mutually
> exclusive with saying before hand that we've diverged from the branch.
> 
>> Maybe improve the doc instead?
> 
> Aside from whether my patch makes any sense, the solution to a UX
> issue really can't be "oh this just needs to be documented". For every
> user who's confused by some interface we provide a *tiny* minority of
> them go and exhaustively read the docs for an explanation, will just
> remain confused.
> 
> I think saying from v.s. at is way too subtle, I for one have been
> missing it for years until this thread, that's bad UX, git's also used
> by a lot of non-native English speakers who may not at all get the
> subtle difference between at and from in this context, or if they do
> think the UI is using that subtlety to tell them something.

Well, we have to find the right balance between clarity and brevity - an
interface that is too chatty is a nightmare. That's why I suggested both
doc changes and additional information.

What do you think about the ahead/behind info as suggested? That should
be more informative both qualitatively (something diverged) and
quantitatively (by how much).

Michael

Re: `git status` output is very misleading after a merge on a "detached HEAD"

[Michael J Gruber]

Enis Bayramoğlu venit, vidit, dixit 12.04.2017 08:15:
> On Wed, Apr 12, 2017 at 8:43 AM, Michael J Gruber <g...@grubix.eu> wrote:
>> Am 11. April 2017 22:40:14 MESZ schrieb "Ævar Arnfjörð Bjarmason" 
>> <ava...@gmail.com>:
>>> On Tue, Apr 11, 2017 at 5:13 PM, Enis Bayramoğlu
>>> <e...@picussecurity.com> wrote:
>>>>> Well, what do you suggest as an alternative?
>>>>>
>>>>> Git tells you that you are in detached state and where you came from
>>>>> (detached from).
>>>>
>>>> I think it'd be best if git status somehow indicated that you're no
>>>> longer at the same commit. Maybe something like:
>>>>
>>>> $ git status
>>>> HEAD detached from origin/master, no longer at the same commit
>>>> nothing to commit, working directory clean
>>>
>>> I'm not saying this is clear, I didn't know this until I read the code
>>> just now, but for what it's worth it says "detached at" if you're
>>> detached from BRANCH but at the same commit, and "detached from" if
>>> you're now on a different commit.
>>>
>>
>> That's what I explained in my first reply which the OP quoted in a chopped 
>> way.  I think he even misquoted the git output he got.
>>
>> It's the difference between from and at.
> 
> You're right, I hadn't noticed the difference, I really thought I
> copied the git output verbatim from the console, but I must've been
> confused while switching windows. Sorry about that.
> 
> I consider myself fairly fluent in English, but it's not my native
> language. If you think head detached "from" vs. "at" is immediately
> and unambiguously clear about what's going on, then I guess it's not
> worth changing the behavior.

I don't mind trying to make this clearer (which is why I asked for
alternatives), I just don't want to reduce the amount of information.
The man pages mentions neither "from" nor "at", so that would be a
starting point.

In fact, I think that "git status [--short]" lacks some of the
information that you only get from, e.g., the git prompt or from "git
checkout" after you switch away. I've set out to amend "git status" with
"in-progress information" (bisecting etc.) in a different thread.

Now for the detached HEAD, I see two variants:

"git branch --list -vv" displays information such as "[origin/next:
ahead 2, behind 3]" for a branch that has diverged from its upstream
"origin/next", and whose upstream has new commits. Mimicking that, git
status could display

HEAD detached from origin/next [ahead 2, behind 3]

either by default or with an extra flag. This requires a "git rev-list
--count --left-right origin/next...HEAD", which is not too bad. The
behind info might be superfluous, but maybe keeping it similar to "git
branch" is beneficial, maybe even more similar:

HEAD detached [from origin/next: ahead 2, behind 3]

"git checkout somewhereElse" display information such as "Warning: you
are leaving 2 commits behind, not connected to any of your branches" and
lists them (the first up to 4). Mimicking that, git status could display

HEAD detached from origin/next [2 orphans]

but this takes more time to compute, especially when you have a lot of
branches. It's the more relevant information in the sense that it counts
those commits that you would loose (once pruned from the reflog) unless
you bind a ref to them or a child commit.

>>
>>
>>>> or, to be more informative
>>>>
>>>> HEAD detached from origin/master 1 commit ago,
>>>
>>> In lieu of that, which would need some of the rev-list machinery to be
>>> invoked on every git-status, I wonder if just saying "HEAD detached &
>>> diverged from origin/master" wouldn't be clearer:
>>>
>>> diff --git a/wt-status.c b/wt-status.c
>>> index 308cf3779e..79c8cfd1cf 100644
>>> --- a/wt-status.c
>>> +++ b/wt-status.c
>>> @@ -1542,7 +1542,7 @@ static void wt_longstatus_print(struct wt_status
>>> *s)
>>>if (state.detached_at)
>>>  on_what = _("HEAD detached at ");
>>>    else
>>> -   on_what = _("HEAD detached from
>>> ");
>>> +   on_what = _("HEAD detached &
>>> diverged from ");
>>>} else {
>>>

Re: `git status` output is very misleading after a merge on a "detached HEAD"

[Michael J Gruber]

Am 11. April 2017 22:40:14 MESZ schrieb "Ævar Arnfjörð Bjarmason" 
<ava...@gmail.com>:
>On Tue, Apr 11, 2017 at 5:13 PM, Enis Bayramoğlu
><e...@picussecurity.com> wrote:
>>> Well, what do you suggest as an alternative?
>>>
>>> Git tells you that you are in detached state and where you came from
>>> (detached from).
>>
>> I think it'd be best if git status somehow indicated that you're no
>> longer at the same commit. Maybe something like:
>>
>> $ git status
>> HEAD detached from origin/master, no longer at the same commit
>> nothing to commit, working directory clean
>
>I'm not saying this is clear, I didn't know this until I read the code
>just now, but for what it's worth it says "detached at" if you're
>detached from BRANCH but at the same commit, and "detached from" if
>you're now on a different commit.
>

That's what I explained in my first reply which the OP quoted in a chopped way. 
 I think he even misquoted the git output he got. 

It's the difference between from and at. 


>> or, to be more informative
>>
>> HEAD detached from origin/master 1 commit ago,
>
>In lieu of that, which would need some of the rev-list machinery to be
>invoked on every git-status, I wonder if just saying "HEAD detached &
>diverged from origin/master" wouldn't be clearer:
>
>diff --git a/wt-status.c b/wt-status.c
>index 308cf3779e..79c8cfd1cf 100644
>--- a/wt-status.c
>+++ b/wt-status.c
>@@ -1542,7 +1542,7 @@ static void wt_longstatus_print(struct wt_status
>*s)
>if (state.detached_at)
>  on_what = _("HEAD detached at ");
>else
>-   on_what = _("HEAD detached from
>");
>+   on_what = _("HEAD detached &
>diverged from ");
>} else {
>branch_name = "";
>   on_what = _("Not currently on any branch.");
>
>
>

No way. That would reduce the information that we give. 

Note that the difference between from and at is also: are there commits that we 
could lose when we switch away, that is: that git checkout would warn us about? 

Maybe improve the doc instead? 

>
>> On Tue, Apr 11, 2017 at 5:55 PM, Michael J Gruber <g...@grubix.eu>
>wrote:
>>> Enis Bayramoğlu venit, vidit, dixit 11.04.2017 10:57:
>>>> I've encountered a very misleading output from `git status`. Here's
>a
>>>> sequence of events that demonstrates the issue:
>>>>
>>>> $ git --version
>>>> git version 2.12.0
>>>>
>>>> $ git checkout origin/master
>>>>
>>>> $ git status
>>>> HEAD detached from origin/master
>>>> nothing to commit, working directory clean
>>>
>>> Hmm. My Git would display "detached at" here as long as you are on
>the
>>> commit that you detached from.
>>>
>>>> $ git merge --ff f3515b749be861b57fc70c2341c1234eeb0d5b87
>>>>
>>>> $ git status
>>>> HEAD detached from origin/master
>>>> nothing to commit, working directory clean
>>>>
>>>> $ git rev-parse origin/master
>>>> e1dc1baaadee0f1aef2d5c45d068306025d11f67
>>>>
>>>> $ git rev-parse HEAD
>>>> 786cb6dd09897e0950a2bdc971f0665a059efd33
>>>>
>>>> I think it's extremely misleading that `git status` simply reports
>>>> "HEAD detached from origin/master" while this simply happens to be
>a
>>>> mildly relevant fact about some past state.
>>>>
>>>> Thanks and regards
>>>>
>>>
>>> Well, what do you suggest as an alternative?
>>>
>>> Git tells you that you are in detached state and where you came from
>>> (detached from).
>>>
>>> Michael

Re: `git status` output is very misleading after a merge on a "detached HEAD"

[Michael J Gruber]

Enis Bayramoğlu venit, vidit, dixit 11.04.2017 10:57:
> I've encountered a very misleading output from `git status`. Here's a
> sequence of events that demonstrates the issue:
> 
> $ git --version
> git version 2.12.0
> 
> $ git checkout origin/master
> 
> $ git status
> HEAD detached from origin/master
> nothing to commit, working directory clean

Hmm. My Git would display "detached at" here as long as you are on the
commit that you detached from.

> $ git merge --ff f3515b749be861b57fc70c2341c1234eeb0d5b87
> 
> $ git status
> HEAD detached from origin/master
> nothing to commit, working directory clean
> 
> $ git rev-parse origin/master
> e1dc1baaadee0f1aef2d5c45d068306025d11f67
> 
> $ git rev-parse HEAD
> 786cb6dd09897e0950a2bdc971f0665a059efd33
> 
> I think it's extremely misleading that `git status` simply reports
> "HEAD detached from origin/master" while this simply happens to be a
> mildly relevant fact about some past state.
> 
> Thanks and regards
> 

Well, what do you suggest as an alternative?

Git tells you that you are in detached state and where you came from
(detached from).

Michael

Re: [PATCH] status: show in-progress info for short status

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 31.03.2017 20:14:
> Michael J Gruber <g...@grubix.eu> writes:
> 
>> Ordinary (long) status shows information about bisect, revert, am,
>> rebase, cherry-pick in progress, and so does git-prompt.sh. status
>> --short currently shows none of this information.
>>
>> Introduce an `--inprogress` argument to git status so that, when used with
>> `--short --branch`, in-progress information is shown next to the branch
>> information. Just like `--branch`, this comes with a config option.
>>
>> The wording for the in-progress information is taken over from
>> git-prompt.sh.
>>
>> Signed-off-by: Michael J Gruber <g...@grubix.eu>
> 
> I haven't formed an opinion on the feature itself, or the way it is
> triggered, so I won't comment on them.  I just say --porcelain (any
> version) may (or may not) want to be extended in backward compatible
> way (but again I haven't formed an opinion on the issue--I just know
> and say there is an issue there that needs to be considered at this
> point).

With my change, "git status --porcelain" output does not change at all
(and neither does that of "git status --short"). They change only when
"--inprogress" (and "--branch") is requested, just like with "--branch".

I don't know the v2 format - it seems that it's built to be extensible,
but I have not checked whether that#s documented somewhere, and I didn't
change it anyways.

>> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
>> index 458608cc1e..103e006249 100755
>> --- a/t/t7512-status-help.sh
>> +++ b/t/t7512-status-help.sh
>> @@ -74,7 +74,6 @@ test_expect_success 'prepare for rebase conflicts' '
>>  
>>  
>>  test_expect_success 'status when rebase in progress before resolving 
>> conflicts' '
>> -test_when_finished "git rebase --abort" &&
>>  ONTO=$(git rev-parse --short HEAD^^) &&
>>  test_must_fail git rebase HEAD^ --onto HEAD^^ &&
>>  cat >expected <> @@ -96,6 +95,15 @@ EOF
>>  test_i18ncmp expected actual
>>  '
>>  
>> +test_expect_success 'short status when rebase in progress' '
>> +test_when_finished "git rebase --abort" &&
>> +cat >expected <> +## HEAD (no branch); REBASE-m
>> +UU main.txt
>> +EOF
>> +git status --untracked-files=no --short --branch --inprogress >actual &&
>> +test_i18ncmp expected actual
>> +'
> 
> This is not a good way to structure the test.  If the one in the
> previous hunk is what creates a conflicted state by running
> "rebase", check the status output from within that test, after the
> conflicting "rebase" fails and other things the existing test checks
> are tested.  That way, you do not have to worry about this new check
> getting confused if the previous one fails in the middle.
> 
> Likewise for the most (if not all---I didn't check very carefully)
> of the remaining hunks in this test script.

All followed the same structure. I did it that way so that it's clearer
which test is failing, but I don't mind putting things together as you
describe. Most of the time, one has to check where in the &&-chain a
test failed, anyways.

Michael

Re: [PATCH] status: show in-progress info for short status

[Michael J Gruber]

SZEDER Gábor venit, vidit, dixit 06.04.2017 16:33:
>> @@ -1779,6 +1780,31 @@ static void wt_shortstatus_print_tracking(struct 
>> wt_status *s)
>>  }
>>  
>>  color_fprintf(s->fp, header_color, "]");
>> +
>> + inprogress:
>> +if (!s->show_inprogress)
>> +goto conclude;
>> +memset(, 0, sizeof(state));
>> +wt_status_get_state(,
>> +s->branch && !strcmp(s->branch, "HEAD"));
>> +if (state.merge_in_progress)
>> +color_fprintf(s->fp, header_color, "; %s", 
>> LABEL(N_("MERGING")));
>> +else if (state.am_in_progress)
>> +color_fprintf(s->fp, header_color, "; %s", LABEL(N_("AM")));
>> +else if (state.rebase_in_progress)
>> +color_fprintf(s->fp, header_color, "; %s", 
>> LABEL(N_("REBASE-m")));
>> +else if (state.rebase_interactive_in_progress)
>> +color_fprintf(s->fp, header_color, "; %s", 
>> LABEL(N_("REBASE-i")));
>> +else if (state.cherry_pick_in_progress)
>> +color_fprintf(s->fp, header_color, "; %s", 
>> LABEL(N_("CHERRY-PICKING")));
>> +else if (state.revert_in_progress)
>> +color_fprintf(s->fp, header_color, "; %s", 
>> LABEL(N_("REVERTING")));
>> +if (state.bisect_in_progress)
> 
> else if?

No. You can do all of the above during a bisect.

> 
>> +color_fprintf(s->fp, header_color, "; %s", 
>> LABEL(N_("BISECTING")));
>> +free(state.branch);
>> +free(state.onto);
>> +free(state.detached_from);
>> +
>>   conclude:
>>  fputc(s->null_termination ? '\0' : '\n', s->fp);
>>  }
> 
> This reminded me of a patch that I have been using for almost two
> years now...
> 
> git-prompt.sh's similar long conditional chain to show the ongoing
> operation has an else-branch at the end showing "AM/REBASE".  Your
> patch doesn't add an equivalent branch.  Is this intentional or an
> oversight?

I go over all states that wt_status_get_state can give.

> I suppose it's intentional, because that "AM/REBASE" branch in the
> prompt seems to be unreachable (see below), but I never took the
> effort to actually check that (hence the "seems" and that's why I
> never submitted it).

Note that wt_status_get_state and the prompt script do things quite
differently.

I suppose that the prompt could make use of the in-progress info being
exposed by "git status" rather doing its on thing.

Michael

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Michael J Gruber]

Duy Nguyen venit, vidit, dixit 25.03.2017 13:07:
> On Fri, Mar 24, 2017 at 12:55 AM, Junio C Hamano <gits...@pobox.com> wrote:
>> Michael J Gruber <g...@drmicha.warpmail.net> writes:
>>
>>> Are we at a point where we can still rename the new feature at least? If
>>> yes, and keeping everything else is mandatory, than "workspace" or
>>> "working space" may be a serious contender for naming the new thing.
>>
>> I do not have a good answer to the first question, but workspace
>> does sound like a good name for what this feature is trying to
>> achieve.
>>
> 
> Now is not too late to rename the command from worktree to workspace
> (and keep "worktree" as an alias that will be eventually deleted).
> Should we do it? I would keep file names, function names... unchanged
> though, not worth the amount of new conflicts.

I guess I would go for a full change. Our technical documentation often
merely consists of the source code, so we should reduce potential
confusion there, too.

Michael

Re: [PATCH v3 3/4] name-rev: provide debug output

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 31.03.2017 20:33:
> Junio C Hamano <gits...@pobox.com> writes:
> 
>> Michael J Gruber <g...@grubix.eu> writes:
>>
>>>> The only case that this change may make a difference I can think of
>>>> is when you have a tag object pointed at from outside refs/tags
>>>> (e.g. refs/heads/foo is a tag object); if you are trying to change
>>>> the definition of "from_tag" from the current "Is the tip inside
>>>> refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag
>>>> object anywhere?", that may be a good change (I didn't think things
>>>> through, though), but that shouldn't be hidden inside a commit that
>>>> claims to only add support for debugging.
>>>>
>>>> What problem are you solving?  
>>>
>>> Sorry, I forgot about that change and failed to mention it.
>>>
>>> It makes no difference in the non-debug case which cares about the
>>> Boolean only. In the debug case, I want to distinguish between
>>> annotated and lightweight tags, just like describe --debug does. By
>>> adding 1 via deref and passing this down, I know that an annotated tag
>>> gets the value 2, a lightweight tag 1 and everything else 0, just like
>>> describe --tags.
>>
>> So it sounds like you meant to do something else, and the
>> implementation is wrong for that something else (i.e. it wouldn't do
>> the right thing for a tag object outside refs/tags/, with or without
>> the "--debug" option passed).
> 
> The damage seems worse, but I may be misreading the code.
> 
> is_better_name() compares name->from_tag and from_tag numerically,
> because it was designed to take a boolean view of that variable.
> Now, an artificially bumped 2 gets compared with name->from_tag that
> may be 1 and gets different priority.  That artificially inflated
> value may be propagated to name->from_tag when the current tip is
> judged as a better basis for naming the object.

No, I checked not to change the existing behaviour.

If you look at the comment above that then you see that one of the sides
of the comparison is a non-tag, so we compare 0 to 1 or 2, the boolean
outcome being the same.

> If this change is only for debugging, perhaps inside if(data->debug)
> you added, instead of looking at from_tag, you can look at both
> from_tag and deref to choose which prio-nmes to show, without
> butchering the value in from_tag variable to affect the existing
> code that is exercised with or without --debug?

What I did overlook, though, was that name-rev uses the notion "under
refs/tags" for "being a tag".

In fact, it's puzzling how different describe and name-rev proceed and
weigh the alternatives. I didn't mean to change that.

In retrospect, displaying the "same" debug information for the two
commands doesn't make too much sense as long as they use different
information. name-rev does-not distinguish between tag types, so why
even display it?

I think I should change 3/4 to display exactly those bits that name-rev
actually uses for weighing different possible descriptions; they are
differents from the "describe-bits". So please withhold 3/4 and 4/4.

Michael

Re: [PATCH v3 3/4] name-rev: provide debug output

[Michael J Gruber]

Am 31. März 2017 18:52:16 MESZ schrieb Junio C Hamano <gits...@pobox.com>:
>Michael J Gruber <g...@grubix.eu> writes:
>
>> Currently, `git describe --contains --debug` does not create any
>debug
>> output because it does not pass the flag down to `git name-rev`,
>which
>> does not know that flag.
>>
>> Teach the latter that flag, so that the former can pass it down (in
>> the following commit).
>>
>> The output is patterned after that of `git describe --debug`, with
>the
>> following differences:
>>
>> describe loops over all args to describe, then over all possible
>> descriptions; name-rev does it the other way round. Therefore, we
>need
>> to amend each possible description by the arg that it is for (and we
>> leave out the "searching to describe" header).
>>
>> The date cut-off for name-rev kicks in way more often than the
>candidate
>> number cut-off of describe, so we do not clutter the output with the
>> cut-off.
>>
>> Signed-off-by: Michael J Gruber <g...@grubix.eu>
>> ---
>
>>  static void name_rev(struct commit *commit,
>>  const char *tip_name, unsigned long taggerdate,
>>  int generation, int distance, int from_tag,
>> -int deref)
>> +int deref, struct name_ref_data *data)
>>  {
>>  struct rev_name *name = (struct rev_name *)commit->util;
>>  struct commit_list *parents;
>> @@ -75,6 +88,7 @@ static void name_rev(struct commit *commit,
>>  
>>  if (deref) {
>>  tip_name = xstrfmt("%s^0", tip_name);
>> +from_tag += 1;
>
>Why this change?  I didn't see it explained in the proposed log
>message.  "deref" is true only when our immediate caller is the one
>that inspected the object at the tip and found it to be a tag object
>(i.e. not a lightweight tag or a branch).  from_tag is about "is the
>tip within refs/tags/ hierarchy?  Yes/No?" and such a caller will
>set it appropriately when calling us.  This function just passes it
>down when it recursively calls itself.  
>
>We shouldn't be mucking with that value ourselves here, should we?
>
>The only case that this change may make a difference I can think of
>is when you have a tag object pointed at from outside refs/tags
>(e.g. refs/heads/foo is a tag object); if you are trying to change
>the definition of "from_tag" from the current "Is the tip inside
>refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag
>object anywhere?", that may be a good change (I didn't think things
>through, though), but that shouldn't be hidden inside a commit that
>claims to only add support for debugging.
>
>What problem are you solving?  

Sorry, I forgot about that change and failed to mention it.

It makes no difference in the non-debug case which cares about the Boolean 
only. In the debug case, I want to distinguish between annotated and 
lightweight tags, just like describe --debug does. By adding 1 via deref and 
passing this down, I know that an annotated tag gets the value 2, a lightweight 
tag 1 and everything else 0, just like describe --tags.

>> @@ -236,7 +273,6 @@ static int name_ref(const char *path, const
>struct object_id *oid, int flags, vo
>>  }
>>  
>>  add_to_tip_table(oid->hash, path, can_abbreviate_output);
>> -
>>  while (o && o->type == OBJ_TAG) {
>>  struct tag *t = (struct tag *) o;
>>  if (!t->tagged)
>
>This is a patch noise we can do without.
>
>Thanks.

[PATCH] status: show in-progress info for short status

[Michael J Gruber]

Ordinary (long) status shows information about bisect, revert, am,
rebase, cherry-pick in progress, and so does git-prompt.sh. status
--short currently shows none of this information.

Introduce an `--inprogress` argument to git status so that, when used with
`--short --branch`, in-progress information is shown next to the branch
information. Just like `--branch`, this comes with a config option.

The wording for the in-progress information is taken over from
git-prompt.sh.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
When used with --porcelain, this gives an easy way to amend the 
powerline-gitstatus
prompt for example (in use locally here), and certainly others.

I don't touch --porcelain=v2 - that one reads in-progress state but does not 
seem
to display it. I don't know which parts are supposed to be stable for v2. 

 Documentation/config.txt |  4 +++
 Documentation/git-status.txt |  5 
 builtin/commit.c | 13 ++
 t/t7512-status-help.sh   | 58 
 wt-status.c  | 32 +---
 wt-status.h  |  1 +
 6 files changed, 105 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d51..3adc65f9d3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2957,6 +2957,10 @@ status.branch::
Set to true to enable --branch by default in linkgit:git-status[1].
The option --no-branch takes precedence over this variable.
 
+status.inprogress::
+   Set to true to enable --inprogress by default in linkgit:git-status[1].
+   The option --no-inprogress takes precedence over this variable.
+
 status.displayCommentPrefix::
If set to true, linkgit:git-status[1] will insert a comment
prefix before each output line (starting with
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..4fac073247 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -32,6 +32,11 @@ OPTIONS
 --branch::
Show the branch and tracking info even in short-format.
 
+-p::
+--inprogress::
+   When showing branch and tracking info in short-format,
+   show in-progress information (BISECTING, MERGING etc.), too.
+
 --porcelain[=]::
Give the output in an easy-to-parse format for scripts.
This is similar to the short output, but will remain stable
diff --git a/builtin/commit.c b/builtin/commit.c
index 4e288bc513..6176dd2c2f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1105,8 +1105,10 @@ static const char *read_commit_message(const char *name)
 static struct status_deferred_config {
enum wt_status_format status_format;
int show_branch;
+   int show_inprogress;
 } status_deferred_config = {
STATUS_FORMAT_UNSPECIFIED,
+   -1, /* unspecified */
-1 /* unspecified */
 };
 
@@ -1133,6 +1135,10 @@ static void finalize_deferred_config(struct wt_status *s)
s->show_branch = status_deferred_config.show_branch;
if (s->show_branch < 0)
s->show_branch = 0;
+   if (use_deferred_config && s->show_inprogress < 0)
+   s->show_inprogress = status_deferred_config.show_inprogress;
+   if (s->show_inprogress < 0)
+   s->show_inprogress = 0;
 }
 
 static int parse_and_validate_options(int argc, const char *argv[],
@@ -1291,6 +1297,10 @@ static int git_status_config(const char *k, const char 
*v, void *cb)
status_deferred_config.show_branch = git_config_bool(k, v);
return 0;
}
+   if (!strcmp(k, "status.inprogress")) {
+   status_deferred_config.show_inprogress = git_config_bool(k, v);
+   return 0;
+   }
if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
s->use_color = git_config_colorbool(k, v);
return 0;
@@ -1339,6 +1349,8 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
N_("show status concisely"), STATUS_FORMAT_SHORT),
OPT_BOOL('b', "branch", _branch,
 N_("show branch information")),
+   OPT_BOOL('p', "inprogress", _inprogress,
+N_("show in-progress information")),
{ OPTION_CALLBACK, 0, "porcelain", _format,
  N_("version"), N_("machine-readable output"),
  PARSE_OPT_OPTARG, opt_parse_porcelain },
@@ -1612,6 +1624,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
OPT_SET_INT(0, "short", _format, N_("show status 
concisely"),
STATUS_FORMAT_SHORT),
OPT_BOOL(0, "branch",

[PATCH v3 3/4] name-rev: provide debug output

[Michael J Gruber]

Currently, `git describe --contains --debug` does not create any debug
output because it does not pass the flag down to `git name-rev`, which
does not know that flag.

Teach the latter that flag, so that the former can pass it down (in
the following commit).

The output is patterned after that of `git describe --debug`, with the
following differences:

describe loops over all args to describe, then over all possible
descriptions; name-rev does it the other way round. Therefore, we need
to amend each possible description by the arg that it is for (and we
leave out the "searching to describe" header).

The date cut-off for name-rev kicks in way more often than the candidate
number cut-off of describe, so we do not clutter the output with the
cut-off.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 Documentation/git-name-rev.txt |  5 
 builtin/name-rev.c | 64 +-
 2 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index e8e68f528c..fd78ee86e8 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -42,6 +42,11 @@ OPTIONS
 --all::
List all commits reachable from all refs
 
+--debug::
+   Verbosely display information about the searching strategy
+   being employed to standard error.  The symbolic name will still
+   be printed to standard out.
+
 --stdin::
Transform stdin by substituting all the 40-character SHA-1
hexes (say $hex) with "$hex ($rev_name)".  When used with
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index bf7ed015ae..51302a79ba 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -18,6 +18,10 @@ typedef struct rev_name {
 
 static long cutoff = LONG_MAX;
 
+static const char *prio_names[] = {
+   N_("head"), N_("lightweight"), N_("annotated"),
+};
+
 /* How many generations are maximally preferred over _one_ merge traversal? */
 #define MERGE_TRAVERSAL_WEIGHT 65535
 
@@ -59,10 +63,19 @@ static int is_better_name(struct rev_name *name,
return 0;
 }
 
+struct name_ref_data {
+   int tags_only;
+   int name_only;
+   int debug;
+   struct string_list ref_filters;
+   struct string_list exclude_filters;
+   struct object_array *revs;
+};
+
 static void name_rev(struct commit *commit,
const char *tip_name, unsigned long taggerdate,
int generation, int distance, int from_tag,
-   int deref)
+   int deref, struct name_ref_data *data)
 {
struct rev_name *name = (struct rev_name *)commit->util;
struct commit_list *parents;
@@ -75,6 +88,7 @@ static void name_rev(struct commit *commit,
 
if (deref) {
tip_name = xstrfmt("%s^0", tip_name);
+   from_tag += 1;
 
if (generation)
die("generation: %d, but deref?", generation);
@@ -87,6 +101,36 @@ static void name_rev(struct commit *commit,
} else if (is_better_name(name, tip_name, taggerdate,
  generation, distance, from_tag)) {
 copy_data:
+   if (data->debug) {
+   int i;
+   static int label_width = -1;
+   static int name_width = -1;
+   if (label_width < 0) {
+   int w;
+   for (i = 0; i < ARRAY_SIZE(prio_names); i++) {
+   w = strlen(_(prio_names[i]));
+   if (label_width < w)
+   label_width = w;
+   }
+   }
+   if (name_width < 0) {
+   int w;
+   for (i = 0; i < data->revs->nr; i++) {
+   w = strlen(data->revs->objects[i].name);
+   if (name_width < w)
+   name_width = w;
+   }
+   }
+   for (i = 0; i < data->revs->nr; i++)
+   if (!oidcmp(>object.oid,
+   >revs->objects[i].item->oid))
+   fprintf(stderr, " %-*s %8d %-*s 
%s~%d\n",
+   label_width,
+   _(prio_names[from_tag]),
+   distance, name_width,
+   data->revs->objects[i].name,
+   tip_name, generation);
+   }
  

[PATCH v3 4/4] describe: pass --debug down to name-rev

[Michael J Gruber]

Now that name-rev knows --debug, pass that flag down to name-rev when we
call it for doing describe --contains.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/describe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/describe.c b/builtin/describe.c
index a5cd8c513f..30196793f0 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -486,6 +486,8 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
 NULL);
if (always)
argv_array_push(, "--always");
+   if (debug)
+   argv_array_push(, "--debug");
if (!all) {
argv_array_push(, "--tags");
for_each_string_list_item(item, )
-- 
2.12.2.739.gfc3eb97820

[PATCH v3 2/4] name-rev: favor describing with tags and use committer date to tiebreak

[Michael J Gruber]

From: Junio C Hamano <gits...@pobox.com>

"git name-rev" assigned a phony "far in the future" date to tips of
refs that are not pointing at tag objects, and favored names based
on a ref with the oldest date.  This made it almost impossible for
an unannotated tags and branches to be counted as a viable base,
which was especially problematic when the command is run with the
"--tags" option.  If an unannotated tag that points at an ancient
commit and an annotated tag that points at a much newer commit
reaches the commit that is being named, the old unannotated tag was
ignored.

Update the "taggerdate" field of the rev-name structure, which is
initialized from the tip of ref, to have the committer date if the
object at the tip of ref is a commit, not a tag, so that we can
optionally take it into account when doing "is this name better?"
comparison logic.

When "name-rev" is run without the "--tags" option, the general
expectation is still to name the commit based on a tag if possible,
but use non-tag refs as fallback, and tiebreak among these non-tag
refs by favoring names with shorter hops from the tip.  The use of a
phony "far in the future" date in the original code was an effective
way to ensure this expectation is held: a non-tag tip gets the same
"far in the future" timestamp, giving precedence to tags, and among
non-tag tips, names with shorter hops are preferred over longer
hops, without taking the "taggerdate" into account.  As we are
taking over the "taggerdate" field to store the committer date for
tips with commits:

 (1) keep the original logic when comparing names based on two refs
 both of which are from refs/tags/;

 (2) favoring a name based on a ref in refs/tags/ hierarchy over
 a ref outside the hierarchy;

 (3) between two names based on a ref both outside refs/tags/, give
 precedence to a name with shorter hops and use "taggerdate"
 only to tie-break.

A change to t4202 is a natural consequence.  The test creates a
commit on a branch "side" and points at it with an unannotated tag
"refs/tags/side-2".  The original code couldn't decide which one to
favor at all, and gave a name based on a branch (simply because
refs/heads/side sorts earlier than refs/tags/side-2).  Because the
updated logic is taught to favor refs in refs/tags/ hierarchy, the
the test is updated to expect to see tags/side-2 instead.

Signed-off-by: Junio C Hamano <gits...@pobox.com>
---

 * I am sure others can add better heurisitics in is_better_name()
   for comparing names based on non-tag refs, and I do not mind
   people disagreeing with the logic that this patch happens to
   implement.  This is done primarily to illustrate the value of
   using a separate helper function is_better_name() instead of
   open-coding the selection logic in name_rev() function.
Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/name-rev.c | 53 -
 t/t4202-log.sh |  2 +-
 2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 7890f826ce..bf7ed015ae 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -13,6 +13,7 @@ typedef struct rev_name {
unsigned long taggerdate;
int generation;
int distance;
+   int from_tag;
 } rev_name;
 
 static long cutoff = LONG_MAX;
@@ -24,16 +25,43 @@ static int is_better_name(struct rev_name *name,
  const char *tip_name,
  unsigned long taggerdate,
  int generation,
- int distance)
+ int distance,
+ int from_tag)
 {
-   return (name->taggerdate > taggerdate ||
-   (name->taggerdate == taggerdate &&
-name->distance > distance));
+   /*
+* When comparing names based on tags, prefer names
+* based on the older tag, even if it is farther away.
+*/
+   if (from_tag && name->from_tag)
+   return (name->taggerdate > taggerdate ||
+   (name->taggerdate == taggerdate &&
+name->distance > distance));
+
+   /*
+* We know that at least one of them is a non-tag at this point.
+* favor a tag over a non-tag.
+*/
+   if (name->from_tag != from_tag)
+   return from_tag;
+
+   /*
+* We are now looking at two non-tags.  Tiebreak to favor
+* shorter hops.
+*/
+   if (name->distance != distance)
+   return name->distance > distance;
+
+   /* ... or tiebreak to favor older date */
+   if (name->taggerdate != taggerdate)
+   return name->taggerdate > taggerdat

[PATCH v3 0/4] name-rev sanity

[Michael J Gruber]

v3 splits the old 3/3 into name-rev and describe related parts and
adds documentation.

No core dumps encountered so far ;)

Did I mention this is on top of mg/describe-debug-l10n (next)?

Junio C Hamano (2):
  name-rev: refactor logic to see if a new candidate is a better name
  name-rev: favor describing with tags and use committer date to
tiebreak

Michael J Gruber (2):
  name-rev: provide debug output
  describe: pass --debug down to name-rev

 Documentation/git-name-rev.txt |   5 ++
 builtin/describe.c |   2 +
 builtin/name-rev.c | 117 +++--
 t/t4202-log.sh |   2 +-
 4 files changed, 108 insertions(+), 18 deletions(-)

-- 
2.12.2.739.gfc3eb97820

[PATCH v3 1/4] name-rev: refactor logic to see if a new candidate is a better name

[Michael J Gruber]

From: Junio C Hamano <gits...@pobox.com>

When we encounter a new ref that could describe the commit we are
looking at, we compare the name that is formed using that ref and
the name we found so far and pick a better one.

Factor the comparison logic out to a separate helper function, while
keeping the current logic the same (i.e. a name that is based on an
older tag is better, and if two tags of the same age can reach the
commit, the one with fewer number of hops to reach the commit is
better).

Signed-off-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/name-rev.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 8bdc3eaa6f..7890f826ce 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -20,6 +20,17 @@ static long cutoff = LONG_MAX;
 /* How many generations are maximally preferred over _one_ merge traversal? */
 #define MERGE_TRAVERSAL_WEIGHT 65535
 
+static int is_better_name(struct rev_name *name,
+ const char *tip_name,
+ unsigned long taggerdate,
+ int generation,
+ int distance)
+{
+   return (name->taggerdate > taggerdate ||
+   (name->taggerdate == taggerdate &&
+name->distance > distance));
+}
+
 static void name_rev(struct commit *commit,
const char *tip_name, unsigned long taggerdate,
int generation, int distance,
@@ -45,9 +56,8 @@ static void name_rev(struct commit *commit,
name = xmalloc(sizeof(rev_name));
commit->util = name;
goto copy_data;
-   } else if (name->taggerdate > taggerdate ||
-   (name->taggerdate == taggerdate &&
-name->distance > distance)) {
+   } else if (is_better_name(name, tip_name, taggerdate,
+ generation, distance)) {
 copy_data:
name->tip_name = tip_name;
name->taggerdate = taggerdate;
-- 
2.12.2.739.gfc3eb97820

Re: [RFC PATCH 0/5] Localise error headers

[Michael J Gruber]

Jeff King venit, vidit, dixit 21.01.2017 15:20:
> On Wed, Jan 11, 2017 at 10:08:46AM -0800, Junio C Hamano wrote:
> 
>> Jeff King  writes:
>>
>>> Yes, I would think die_errno() is a no-brainer for translation, since
>>> the strerror() will be translated.
>>>
 apply.c:die(_("internal error"));

 That is funny, too. I think we should substitute that with

 die("BUG: untranslated, but what went wrong instead")
>>>
>>> Yep. We did not consistently use "BUG:" in the early days. I would say
>>> that "BUG" lines do not need to be translated. The point is that nobody
>>> should ever see them, so it seems like there is little point in giving
>>> extra work to translators.
>>
>> In addition, "BUG: " is relatively recent introduction to our
>> codebase.  Perhaps having a separate BUG() function help the
>> distinction further?
> 
> Yes, I think so. I have often been tempted to dump core on BUGs for
> further analysis. You can do that by string-matching "BUG:" from the
> beginning of a die message, but it's kind of gross. :)
> 
> -Peff

I read back the whole thread, and I'm still not sure if there's
consensus and how to go forward. Should we let the topic die? I don't
care too much personally, I just thought the mixed tranlations look
"unprofessional".

Michael

Re: [PATCH v2 0/3] name-rev sanity

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 29.03.2017 19:43:
> Junio C Hamano  writes:
> 
>> The first two applies cleanly to the same base as jc/name-rev that
>> the first two of these patches are meant to replace, but the third
>> one doesn't apply on top.  Are you depending on something newer?
> 
> Ah, of course, you are depending on your other topic ;-)
> I'll wiggle these in.
> 
> Thanks.
> 

Yes, sorry, isn't that in next already? I should have meantioned it anyways.

Also, I should split patch 3 into name-rev and describe related parts
and update the name-rev documentation. We don't have tests for describe
--debug, that should be ok.

Mihael

[PATCH v2 0/3] name-rev sanity

[Michael J Gruber]

So here is v2 of the name-rev series, the result of our discussions being:

Junio C Hamano (2):
  name-rev: refactor logic to see if a new candidate is a better name
  name-rev: favor describing with tags and use committer date to
tiebreak

That second patch is slighty changed as discussed, but still mostly Junio's

Michael J Gruber (1):
  name-rev: provide debug output

This replaces the patch which documented that --debug does not work with 
--contains :)

 builtin/describe.c |   2 +
 builtin/name-rev.c | 117 +
 t/t4202-log.sh |   2 +-
 3 files changed, 103 insertions(+), 18 deletions(-)

-- 
2.12.2.712.gba4c48c431

[PATCH v2 2/3] name-rev: favor describing with tags and use committer date to tiebreak

[Michael J Gruber]

From: Junio C Hamano <gits...@pobox.com>

"git name-rev" assigned a phony "far in the future" date to tips of
refs that are not pointing at tag objects, and favored names based
on a ref with the oldest date.  This made it almost impossible for
an unannotated tags and branches to be counted as a viable base,
which was especially problematic when the command is run with the
"--tags" option.  If an unannotated tag that points at an ancient
commit and an annotated tag that points at a much newer commit
reaches the commit that is being named, the old unannotated tag was
ignored.

Update the "taggerdate" field of the rev-name structure, which is
initialized from the tip of ref, to have the committer date if the
object at the tip of ref is a commit, not a tag, so that we can
optionally take it into account when doing "is this name better?"
comparison logic.

When "name-rev" is run without the "--tags" option, the general
expectation is still to name the commit based on a tag if possible,
but use non-tag refs as fallback, and tiebreak among these non-tag
refs by favoring names with shorter hops from the tip.  The use of a
phony "far in the future" date in the original code was an effective
way to ensure this expectation is held: a non-tag tip gets the same
"far in the future" timestamp, giving precedence to tags, and among
non-tag tips, names with shorter hops are preferred over longer
hops, without taking the "taggerdate" into account.  As we are
taking over the "taggerdate" field to store the committer date for
tips with commits:

 (1) keep the original logic when comparing names based on two refs
 both of which are from refs/tags/;

 (2) favoring a name based on a ref in refs/tags/ hierarchy over
 a ref outside the hierarchy;

 (3) between two names based on a ref both outside refs/tags/, give
 precedence to a name with shorter hops and use "taggerdate"
 only to tie-break.

A change to t4202 is a natural consequence.  The test creates a
commit on a branch "side" and points at it with an unannotated tag
"refs/tags/side-2".  The original code couldn't decide which one to
favor at all, and gave a name based on a branch (simply because
refs/heads/side sorts earlier than refs/tags/side-2).  Because the
updated logic is taught to favor refs in refs/tags/ hierarchy, the
the test is updated to expect to see tags/side-2 instead.

Signed-off-by: Junio C Hamano <gits...@pobox.com>
---

 * I am sure others can add better heurisitics in is_better_name()
   for comparing names based on non-tag refs, and I do not mind
   people disagreeing with the logic that this patch happens to
   implement.  This is done primarily to illustrate the value of
   using a separate helper function is_better_name() instead of
   open-coding the selection logic in name_rev() function.
Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/name-rev.c | 53 -
 t/t4202-log.sh |  2 +-
 2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 7890f826ce..bf7ed015ae 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -13,6 +13,7 @@ typedef struct rev_name {
unsigned long taggerdate;
int generation;
int distance;
+   int from_tag;
 } rev_name;
 
 static long cutoff = LONG_MAX;
@@ -24,16 +25,43 @@ static int is_better_name(struct rev_name *name,
  const char *tip_name,
  unsigned long taggerdate,
  int generation,
- int distance)
+ int distance,
+ int from_tag)
 {
-   return (name->taggerdate > taggerdate ||
-   (name->taggerdate == taggerdate &&
-name->distance > distance));
+   /*
+* When comparing names based on tags, prefer names
+* based on the older tag, even if it is farther away.
+*/
+   if (from_tag && name->from_tag)
+   return (name->taggerdate > taggerdate ||
+   (name->taggerdate == taggerdate &&
+name->distance > distance));
+
+   /*
+* We know that at least one of them is a non-tag at this point.
+* favor a tag over a non-tag.
+*/
+   if (name->from_tag != from_tag)
+   return from_tag;
+
+   /*
+* We are now looking at two non-tags.  Tiebreak to favor
+* shorter hops.
+*/
+   if (name->distance != distance)
+   return name->distance > distance;
+
+   /* ... or tiebreak to favor older date */
+   if (name->taggerdate != taggerdate)
+   return name->taggerdate > taggerdat

[PATCH v2 3/3] name-rev: provide debug output

[Michael J Gruber]

Currently, `git describe --contains --debug` does not create any debug
output because it does not pass the flag down to `git name-rev`, which
does not know that flag.

Teach the latter that flag and the former to pass it down.

The output is patterned after that of `git describe --debug`, with the
following differences:

describe loops over all args to describe, then over all possible
descriptions; name-rev does it the other way round. Therefore, we need
to amend each possible description by the arg that it is for (and we
leave out the "searching to describe" header).

The date cut-off for name-rev kicks in way more often than the candidate
number cut-off of describe, so we do not clutter the output with the
cut-off.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/describe.c |  2 ++
 builtin/name-rev.c | 64 +++---
 2 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index a5cd8c513f..30196793f0 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -486,6 +486,8 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
 NULL);
if (always)
argv_array_push(, "--always");
+   if (debug)
+   argv_array_push(, "--debug");
if (!all) {
argv_array_push(, "--tags");
for_each_string_list_item(item, )
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index bf7ed015ae..51302a79ba 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -18,6 +18,10 @@ typedef struct rev_name {
 
 static long cutoff = LONG_MAX;
 
+static const char *prio_names[] = {
+   N_("head"), N_("lightweight"), N_("annotated"),
+};
+
 /* How many generations are maximally preferred over _one_ merge traversal? */
 #define MERGE_TRAVERSAL_WEIGHT 65535
 
@@ -59,10 +63,19 @@ static int is_better_name(struct rev_name *name,
return 0;
 }
 
+struct name_ref_data {
+   int tags_only;
+   int name_only;
+   int debug;
+   struct string_list ref_filters;
+   struct string_list exclude_filters;
+   struct object_array *revs;
+};
+
 static void name_rev(struct commit *commit,
const char *tip_name, unsigned long taggerdate,
int generation, int distance, int from_tag,
-   int deref)
+   int deref, struct name_ref_data *data)
 {
struct rev_name *name = (struct rev_name *)commit->util;
struct commit_list *parents;
@@ -75,6 +88,7 @@ static void name_rev(struct commit *commit,
 
if (deref) {
tip_name = xstrfmt("%s^0", tip_name);
+   from_tag += 1;
 
if (generation)
die("generation: %d, but deref?", generation);
@@ -87,6 +101,36 @@ static void name_rev(struct commit *commit,
} else if (is_better_name(name, tip_name, taggerdate,
  generation, distance, from_tag)) {
 copy_data:
+   if (data->debug) {
+   int i;
+   static int label_width = -1;
+   static int name_width = -1;
+   if (label_width < 0) {
+   int w;
+   for (i = 0; i < ARRAY_SIZE(prio_names); i++) {
+   w = strlen(_(prio_names[i]));
+   if (label_width < w)
+   label_width = w;
+   }
+   }
+   if (name_width < 0) {
+   int w;
+   for (i = 0; i < data->revs->nr; i++) {
+   w = strlen(data->revs->objects[i].name);
+   if (name_width < w)
+   name_width = w;
+   }
+   }
+   for (i = 0; i < data->revs->nr; i++)
+   if (!oidcmp(>object.oid,
+   >revs->objects[i].item->oid))
+   fprintf(stderr, " %-*s %8d %-*s 
%s~%d\n",
+   label_width,
+   _(prio_names[from_tag]),
+   distance, name_width,
+   data->revs->objects[i].name,
+   tip_name, generation);
+   }
name->tip_name = tip_name;
name->tagge

[PATCH v2 1/3] name-rev: refactor logic to see if a new candidate is a better name

[Michael J Gruber]

From: Junio C Hamano <gits...@pobox.com>

When we encounter a new ref that could describe the commit we are
looking at, we compare the name that is formed using that ref and
the name we found so far and pick a better one.

Factor the comparison logic out to a separate helper function, while
keeping the current logic the same (i.e. a name that is based on an
older tag is better, and if two tags of the same age can reach the
commit, the one with fewer number of hops to reach the commit is
better).

Signed-off-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/name-rev.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 8bdc3eaa6f..7890f826ce 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -20,6 +20,17 @@ static long cutoff = LONG_MAX;
 /* How many generations are maximally preferred over _one_ merge traversal? */
 #define MERGE_TRAVERSAL_WEIGHT 65535
 
+static int is_better_name(struct rev_name *name,
+ const char *tip_name,
+ unsigned long taggerdate,
+ int generation,
+ int distance)
+{
+   return (name->taggerdate > taggerdate ||
+   (name->taggerdate == taggerdate &&
+name->distance > distance));
+}
+
 static void name_rev(struct commit *commit,
const char *tip_name, unsigned long taggerdate,
int generation, int distance,
@@ -45,9 +56,8 @@ static void name_rev(struct commit *commit,
name = xmalloc(sizeof(rev_name));
commit->util = name;
goto copy_data;
-   } else if (name->taggerdate > taggerdate ||
-   (name->taggerdate == taggerdate &&
-name->distance > distance)) {
+   } else if (is_better_name(name, tip_name, taggerdate,
+ generation, distance)) {
 copy_data:
name->tip_name = tip_name;
name->taggerdate = taggerdate;
-- 
2.12.2.712.gba4c48c431

[PATCH v2 0/2] describe: localize debug output

[Michael J Gruber]

v2 computes the width for the localized output dynamically.
In fact, it might overcalculated a bit depending on the encoding,
but this does not do any harm.

Michael J Gruber (2):
  describe: localize debug output fully
  l10n: de: translate describe debug terms

 builtin/describe.c | 15 ---
 po/de.po   | 14 +-
 2 files changed, 25 insertions(+), 4 deletions(-)

-- 
2.12.2.584.g7becbf139a

[PATCH v2 2/2] l10n: de: translate describe debug terms

[Michael J Gruber]

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 po/de.po | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/po/de.po b/po/de.po
index e9c86f5488..913db393dc 100644
--- a/po/de.po
+++ b/po/de.po
@@ -7530,7 +7530,19 @@ msgstr "git describe [] [...]"
 msgid "git describe [] --dirty"
 msgstr "git describe [] --dirty"
 
-#: builtin/describe.c:217
+#: builtin/describe.c:52
+msgid "head"
+msgstr "Branch"
+
+#: builtin/describe.c:52
+msgid "lightweight"
+msgstr "nicht-annotiert"
+
+#: builtin/describe.c:52
+msgid "annotated"
+msgstr "annotiert"
+
+#: builtin/describe.c:249
 #, c-format
 msgid "annotated tag %s not available"
 msgstr "annotiertes Tag %s ist nicht verfügbar"
-- 
2.12.2.584.g7becbf139a

[PATCH v2 1/2] describe: localize debug output fully

[Michael J Gruber]

git describe --debug localizes all debug messages but not the terms
head, lightweight, annotated that it outputs for the candidates.
Localize them, too.

Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 builtin/describe.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 45adbf67d5..99e963dfe7 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -50,7 +50,7 @@ struct commit_name {
 };
 
 static const char *prio_names[] = {
-   "head", "lightweight", "annotated",
+   N_("head"), N_("lightweight"), N_("annotated"),
 };
 
 static int commit_name_cmp(const struct commit_name *cn1,
@@ -395,10 +395,19 @@ static void describe(const char *arg, int last_one)
free_commit_list(list);
 
if (debug) {
+   static int label_width = -1;
+   if (label_width < 0) {
+   int i, w;
+   for (i = 0; i < ARRAY_SIZE(prio_names); i++) {
+   w = strlen(_(prio_names[i]));
+   if (label_width < w)
+   label_width = w;
+   }
+   }   
for (cur_match = 0; cur_match < match_cnt; cur_match++) {
struct possible_tag *t = _matches[cur_match];
-   fprintf(stderr, " %-11s %8d %s\n",
-   prio_names[t->name->prio],
+   fprintf(stderr, " %-*s %8d %s\n",
+   label_width, _(prio_names[t->name->prio]),
t->depth, t->name->path);
}
fprintf(stderr, _("traversed %lu commits\n"), seen_commits);
-- 
2.12.2.584.g7becbf139a

Re: report on a possible bug: git commit -p myfile.py unexpected output

[Michael J Gruber]

Joan Aguilar venit, vidit, dixit 24.03.2017 11:27:
> Hello there
> 
> this is the first bug report of my life and I am not a native English
> speaker so, first of all, I would like to apologize for my English
> skills and the report itself (if it is not precise enough).
> 
> I have already read this 'guidelines'
> http://www.chiark.greenend.org.uk/~sgtatham/bugs.html and I will try
> to attach to them as much as I can.
> 
> What System I am running:
> * Linux 4.9.0-1-amd64 #1 SMP Debian 4.9.6-3 (2017-01-28) x86_64 GNU/Linux
> * git version 2.11.0. Well actually, according to the apt-get install
> output (of course after apt-get update) -> git is already the newest
> version (1:2.11.0-2).
> * VIM - Vi IMproved 8.0 (2016 Sep 12, compiled Feb 13 2017 00:56:16)
> Included patches: 1-197, 322
> 
> What happened:
> I was working on my git repository yesterday and did a lot of changes
> in the file myfile.py. After a little bit of testing I was ready to
> commit them. I decided not to commit all of them at once (because
> these are non related changes), but instead I decided to use the -p
> option (as I always do in this case). For the first patch I decided to
> commit only all methods I have removed because these are not needed
> anymore. So my Idea was to jump from hunk to hunk and split them
> (using 's') to select (with 'y') only the parts I wanted to commit,
> ie, lines I had removed because they are not useful for me anymore. So
> I went for it and I did:
> 
> git commit -p myfile.py
> 
> Until this point all went as expected. The first hunk was printed in
> the console and the it waited for my decision. As always. After a
> couple of hunks I realized it was not possible to split all of them in
> a proper way (to select only removed lines) because new methods were
> mixed up with the old ones. So I cancel the process (ctrl + c) and
> started again using 'e' instead to manually edit the hunks.
> 
> So I repeated the last command: git commit -p myfile.py but this time
> I only used 'y', 'n' and 'e'. All in all I edited ('e') two big hunks.
> By doing this, the default editor (vim in my case) opened showing the
> hunk and with the instructions in how to edit at the bottom.
> Everything as used to be. Until this point all went as I expected.
> 
> In the two hunk I edited I did the same. I removed all + lines by
> deleting them and I make some - lines as context. Like explained in
> the bottom of the file.
> 
> When I was done with each of the hunks, I saved and closed the editor.
> No error was produced (I sometimes make mistakes while editing a hunk
> and know you can get an error (patch does not apply) here, but in this
> case were no errors there so the next hunk was printed and the commit
> procedure kept going.
> 
> When I was done with all hunks, the editor (vim) started again to
> write the commit message. I wrote (something like) this:
> 
> myfile.py -> old unused methods removed...
> 
> 1) mymethod1
> 2) mymethod2
> 3) mymethod3
> 4) mymethod4
> 5) mymethod5
> 
> Yes, not the best commit ever ;) but enough for us in this case.
> Anyway, I saved it and close the editor and I was surprised by the git
> output.
> 
> [master 96d1c24] myfile.py -> old unused methods removed...
>  1 file changed, 182 insertions(+), 302 deletions(-)
>  rewrite myfile.py (60%)
> 
> What?? I thought to myself... Insertions?? This can't be true? I
> removed all + lines, my idea was to commit only deletions... What did
> I wrong?
> 
> To check this I did
> * git show 96d1c24
> and I saw only red minus lines , ie, deletions. As I expected.

What dows "git show --stat" say?

> To check it twice I used tig too.
> 
> tig showed the same for this specific commit
> myfile.py | 120
> -
> 1 file changed, 120 deletions(-)
> 
> Conclusion:
> After these two checks I am sure I did what I intended to do. What I
> do not understand is the output of git when I was done with the
> commit.
> 
> Maybe is just me, because I do not understand how git commit -p when
> using 'e' to edit hunks works. But as user I would expect to see only
> deletions in the output message if I am picking only deletions.
> 
> If this is a bug, I hope you can reproduce it on your system. If not,
> do not hesitate to contact me for further information.
> 
> 
> Kind regards,
> 
> Joan Aguilar
> --
> Joan Aguilar Lorente
> 

182-302 = -120

Did you make any changes in the lines that you left? Apparantly, that's
what the rewrite looked like to git commit.

Michael

Re: [PATCH v3 2/2] l10n: Add git-add.txt to localized man pages

[Michael J Gruber]

Stefan Beller venit, vidit, dixit 22.03.2017 19:59:
> On Wed, Mar 22, 2017 at 11:56 AM, Junio C Hamano  wrote:
>>> So we'd want to be able to say:
>>>   "get a tarball including all submodules except the superproject"
>>>   (This would produce the "optional language pack tarball")
>>
>> You do not need that.  Just go to the gitman-l10n project and grab a
>> tarball out of it.
> 
> Oh, I misunderstood your proposal.
> You said: We have *one* submodule for all languages, but I understood
> we'd have a submodule for *each* language.

In general, submodules would remove the major gripe that I have with the
current organization, that is carrying out-of-date translations in tree.
submodules make it clear that git.git refers to a specific revision of
the translations.

Now, since not even git.pot is insync with the l10n mark-up in the code
base, I'm afraid everything in po qualifies for being externalized.
Junio's current "pull l10n" would be substituted by updating the l10n
submodule version that git.git references.

In turn, the l10n coordinator may want to update submodule versions for
each language rather than pulling updates. That would allow the space
savings for the common uni- or bilingual developper that we are after.
Recursive submodules, yeah ;)

I'm unsure whether we should/can treat translations of git and the man
pages the same. I tend to say yes (being unsure about the consequences),
as I would hope that translators would be the same so that we keep
consistency across several tranlations in one language.

Michael

[PATCH] mailmap: use Michael J Gruber's new address

[Michael J Gruber]

Map both old addresses to the new, hopefully more permanent one.

Signed-off-by: Michael J Gruber <g...@drmicha.warpmail.net>
Signed-off-by: Michael J Gruber <g...@grubix.eu>
---
 .mailmap | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index e06526a493..ab85e0d16d 100644
--- a/.mailmap
+++ b/.mailmap
@@ -151,7 +151,8 @@ Matthias Kestenholz <matth...@spinlock.ch> 
<m...@spinlock.ch>
 Matthias Urlichs <matth...@urlichs.de> <smurf@kiste.(none)>
 Matthias Urlichs <matth...@urlichs.de> <sm...@smurf.noris.de>
 Michael Coleman <tutu...@gmail.com>
-Michael J Gruber <g...@drmicha.warpmail.net> <michaeljgruber+gm...@fastmail.fm>
+Michael J Gruber <g...@grubix.eu> <michaeljgruber+gm...@fastmail.fm>
+Michael J Gruber <g...@grubix.eu> <g...@drmicha.warpmail.net>
 Michael S. Tsirkin <m...@kernel.org> <m...@redhat.com>
 Michael S. Tsirkin <m...@kernel.org> <m...@mellanox.co.il>
 Michael S. Tsirkin <m...@kernel.org> <m...@dev.mellanox.co.il>
-- 
2.12.1.598.g142b194aba

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 21.03.2017 16:48:
> Duy Nguyen  writes:
> 
>> On Tue, Mar 21, 2017 at 1:50 AM, Jonathan Nieder  wrote:
>>> Junio C Hamano wrote:
 Stefan Beller  writes:
>>>
> While it may be true that you can have bare worktrees; I would question
> why anyone wants to do this, as the only thing it provides is an
> additional HEAD (plus its reflog).

 A more plausible situation is you start with a bare one as the
 primary and used to make local clones to do your work in the world
 before "git worktree".  It would be a natural extension to your
 workflow to instead create worktrees of of that bare one as the
 primary worktree with secondaries with working trees.
>>>
>>> For what it's worth, this conversation makes me think it was a mistake
>>> to call this construct a worktree.
>>
>> For the record, I am totally confused with Junio's last line, with two
>> "with"s, "worktree" and "working trees" in the same phrase :D
> 
> In case this wasn't just a tangential note, what I meant was:
> 
>  - In the old world, you may have had a single bare repository and
>then made clones, each of which has a working tree (i.e. non-bare
>clones), and worked inside these clones.
> 
>  - In the "git worktree" world, you can start from that same single
>bare repository, but instead of cloning it, use "git worktree" to
>create "worktree"s, each of which has a working tree, and work
>inside these "worktree"s.
> 
> and the latter would be a natural extension to the workflow the
> former wanted to use.
> 
>>> It's fine for the command to have one name and the documentation to
>>> use a longer, clearer name to explain it.  What should that longer,
>>> clearer name be?
>>
>> No comments from me. I'll let you know that if Eric (or Junio?) didn't
>> stop me, we would have had $GIT_DIR/repos now instead of
>> $GIT_DIR/worktrees, just some extra confusion toppings.
> 
> I forgot about that part of the history, but you are saying you
> wanted to call these "repos", not "worktrees"?  I can see why
> somebody (or me?) would stop that by fearing "repo" is a bit too
> confusing with a "repository", in the same way that we are now
> realizing that "worktree" is too similar to an old synonym we used
> to call "working tree".
> 

I would say the new thing is really a "checkout", but that opens another
can of worms. On the other hand, "git checkout" already does:
- switching of branches
- creation of branches
- detaching of head
- partial updates of the working tree
So why shouldn't it manage worktrees, as well?

While that may sound a bit sarcastic it indicates that we may want to
rethink some things at some point rather than adding up to the
conflation. The discussion in this thread seems to show that "worktree"
is just as a good a name for the new feature, while "workbase" or
"workroot" (or "workdir") or so could have been for the old one.

Are we at a point where we can still rename the new feature at least? If
yes, and keeping everything else is mandatory, than "workspace" or
"working space" may be a serious contender for naming the new thing.

Michael

[PATCH 1/2] describe: localize debug output fully

[Michael J Gruber]

git describe --debug localizes all debug messages but not the terms
head, lightweight, annotated that it outputs for the candidates.
Localize them, too.

Also, increase the width of that field to create room for the translated
terms.

Signed-off-by: Michael J Gruber <g...@drmicha.warpmail.net>
---
Ralf: this is just the context for the following l10-de patch

 builtin/describe.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 76c18059bf..1a760c16f9 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -49,7 +49,7 @@ struct commit_name {
 };
 
 static const char *prio_names[] = {
-   "head", "lightweight", "annotated",
+   N_("head"), N_("lightweight"), N_("annotated"),
 };
 
 static int commit_name_cmp(const struct commit_name *cn1,
@@ -396,8 +396,8 @@ static void describe(const char *arg, int last_one)
if (debug) {
for (cur_match = 0; cur_match < match_cnt; cur_match++) {
struct possible_tag *t = _matches[cur_match];
-   fprintf(stderr, " %-11s %8d %s\n",
-   prio_names[t->name->prio],
+   fprintf(stderr, " %-15s %8d %s\n",
+   _(prio_names[t->name->prio]),
t->depth, t->name->path);
}
fprintf(stderr, _("traversed %lu commits\n"), seen_commits);
-- 
2.12.0.484.g92f9ab2bc1

[PATCH 2/2] l10n: de: translate describe debug terms

[Michael J Gruber]

Signed-off-by: Michael J Gruber <g...@drmicha.warpmail.net>
---
Junio: this is just a l10n-followup to the previous code patch ;)

 po/de.po | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/po/de.po b/po/de.po
index e9c86f5488..913db393dc 100644
--- a/po/de.po
+++ b/po/de.po
@@ -7530,7 +7530,19 @@ msgstr "git describe [] [...]"
 msgid "git describe [] --dirty"
 msgstr "git describe [] --dirty"
 
-#: builtin/describe.c:217
+#: builtin/describe.c:52
+msgid "head"
+msgstr "Branch"
+
+#: builtin/describe.c:52
+msgid "lightweight"
+msgstr "nicht-annotiert"
+
+#: builtin/describe.c:52
+msgid "annotated"
+msgstr "annotiert"
+
+#: builtin/describe.c:249
 #, c-format
 msgid "annotated tag %s not available"
 msgstr "annotiertes Tag %s ist nicht verfügbar"
-- 
2.12.0.484.g92f9ab2bc1

[PATCH] l10n: de: lower case after semi-colon

[Michael J Gruber]

Signed-off-by: Michael J Gruber <g...@drmicha.warpmail.net>
---
Just a minor thing. I'm wondering about lower/upper case
at the beginning of the line, though. Do we have a rule for de.po?

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

diff --git a/po/de.po b/po/de.po
index e9c86f5488..f8215945e7 100644
--- a/po/de.po
+++ b/po/de.po
@@ -7599,7 +7599,7 @@ msgid ""
 "more than %i tags found; listed %i most recent\n"
 "gave up search at %s\n"
 msgstr ""
-"mehr als %i Tags gefunden; Führe die ersten %i auf\n"
+"mehr als %i Tags gefunden; führe die ersten %i auf\n"
 "Suche bei %s aufgegeben\n"
 
 #: builtin/describe.c:396
-- 
2.12.0.484.g92f9ab2bc1

Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

[Michael J Gruber]

> hops, without taking the "taggerdate" into account.  As we are
> taking over the "taggerdate" field to store the committer date for
> tips with commits:
> 
>  (1) keep the original logic when comparing names based on two refs
>  both of which are from refs/tags/;
> 
>  (2) favoring a name based on a ref in refs/tags/ hierarchy over
>  a ref outside the hierarchy;
> 
>  (3) between two names based on a ref both outside refs/tags/, give
>  precedence to a name with shorter hops and use "taggerdate"
>  only to tie-break.
> 
> A change to t4202 is a natural consequence.  The test creates a
> commit on a branch "side" and points at it with an unannotated tag
> "refs/tags/side-2".  The original code couldn't decide which one to
> favor at all, and gave a name based on a branch (simply because
> refs/heads/side sorts earlier than refs/tags/side-2).  Because the
> updated logic is taught to favor refs in refs/tags/ hierarchy, the
> the test is updated to expect to see tags/side-2 instead.
> 
> Signed-off-by: Junio C Hamano 

I think that strategy is fine and works as I expected in all tested
cases. Thanks!

> ---
> 
>  * I am sure others can add better heurisitics in is_better_name()
>for comparing names based on non-tag refs, and I do not mind
>people disagreeing with the logic that this patch happens to
>implement.  This is done primarily to illustrate the value of
>using a separate helper function is_better_name() instead of
>open-coding the selection logic in name_rev() function.
> ---
>  builtin/name-rev.c | 57 
> +-
>  t/t4202-log.sh |  2 +-
>  2 files changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index f64c71d9bc..eac0180c62 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -13,6 +13,7 @@ typedef struct rev_name {
>   unsigned long taggerdate;
>   int generation;
>   int distance;
> + int from_tag;
>  } rev_name;
>  
>  static long cutoff = LONG_MAX;
> @@ -24,16 +25,47 @@ static int is_better_name(struct rev_name *name,
> const char *tip_name,
> unsigned long taggerdate,
> int generation,
> -   int distance)
> +   int distance,
> +   int from_tag)
>  {
> - return (name->taggerdate > taggerdate ||
> - (name->taggerdate == taggerdate &&
> -  name->distance > distance));
> + /*
> +  * When comparing names based on tags, prefer names
> +  * based on the older tag, even if it is farther away.
> +  */
> + if (from_tag && name->from_tag)
> + return (name->taggerdate > taggerdate ||
> + (name->taggerdate == taggerdate &&
> +  name->distance > distance));
> +
> +#define COMPARE(attribute, smaller_is_better) \
> + if (name->attribute > attribute) \
> + return smaller_is_better; \
> + if (name->attribute < attribute) \
> + return !smaller_is_better

I find that define pretty confusing. On first reading, the "==" case
seems to be missing, but that is basically "pass" as one can see in the
later code.

Also, the comparison ">"  and "<" is used to switch between the cases
"tag vs. non-tag" and "non-tag vs. tag" which happen to be encoded by
the from_tag attribute beeing "1 vs. 0" resp "0 vs. 1" in the following:

> +
> + /*
> +  * We know that at least one of them is a non-tag at this point.
> +  * favor a tag over a non-tag.
> +  */
> + COMPARE(from_tag, 0);
> +

But in the next two instances you use it to do "actual" comparisons
between distances and dates:

> + /*
> +  * We are now looking at two non-tags.  Tiebreak to favor
> +  * shorter hops.
> +  */
> + COMPARE(distance, 1);
> +
> + /* ... or tiebreak to favor older date */
> + COMPARE(taggerdate, 1);
> +
> + /* keep the current one if we cannot decide */
> + return 0;
> +#undef COMPARE
>  }

So, while I do understand that now, I'm wondering whether I will next
time ;)

How about something like

/* favor tag over non-tag */
if (name->from_tag != from_tag)
return from_tag;

at least for the first instance and possibly

/* favor shorter hops for non-tags */
if (name->distance != distance)
return name->distance > distance;

/* tie-break by date */
if (name->taggerdate != taggerdate)
return name->taggerdate > taggerdate;

>  
>  static void name_rev(struct commit *commit,
>   const char *tip_name, unsigned long taggerdate,
> - int generation, int distance,
> + int generation, int distance, int from_tag,
>   int deref)
>  {
>   struct rev_name *name = (struct rev_name *)commit->util;
> @@ -57,12 +89,13 @@ static void name_rev(struct commit *commit,
>   

Re: [PATCH 1/3] describe: debug is incompatible with contains

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 15.03.2017 20:21:
> Michael J Gruber <g...@drmicha.warpmail.net> writes:
> 
>> `git describe --contains` calls into `git name-rev` which does not have
>> any searching to do and thus does not display any debug information.
>>
>> Say so in the documentation and catch the incompatible arguments.
> 
> I am not sure if this is worth it.  Those who are really doing the
> debugging would be staring at the code while running it anyway, so
> it is not like this new error condition would help anybody from
> wasting time scratching her head before viewing the source and
> realize that the underlying name-rev does not honor the option.

The story was: I tried to understand why git describe --contains did not
do what I expected. The documentation said that --debug would output
candidates, but it did not do anything.

So, instead of learning how git describe --contains works, I got
side-tracked into understanding why git describe --contains does not do
what the documentation says. That was a waste of time that we can avoid.

"viewing the source" should not be necessary to understand what is going
on, should it?

> If "--debug" is truly valuable, "name-rev" can gain "--debug" later
> and then we can pass it down if we want.
> 
> Also, it is not like "--debug" is incompatible.  It is just the
> "--contains" codepath is overly silent and does not give any useful
> information when run in the debug mode.  "incompatible" is more like
> "would not work correctly when both are given", which is not the
> case here.

Well, thee notion of giving debug output is certainly not incompatible.
But if the "--debug" does not output anything with "--contains" then it
is not working, which I would call incompatible (implementation, not
concept).

>>
>> Signed-off-by: Michael J Gruber <g...@drmicha.warpmail.net>
>> ---
>>  Documentation/git-describe.txt | 2 +-
>>  builtin/describe.c | 2 ++
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
>> index 8755f3af7b..0f9adb6e9a 100644
>> --- a/Documentation/git-describe.txt
>> +++ b/Documentation/git-describe.txt
>> @@ -69,7 +69,7 @@ OPTIONS
>>  --debug::
>>  Verbosely display information about the searching strategy
>>  being employed to standard error.  The tag name will still
>> -be printed to standard out.
>> +be printed to standard out. This is incompatible with --contains.
>>  
>>  --long::
>>  Always output the long format (the tag, the number of commits
>> diff --git a/builtin/describe.c b/builtin/describe.c
>> index 76c18059bf..01a6d159a0 100644
>> --- a/builtin/describe.c
>> +++ b/builtin/describe.c
>> @@ -462,6 +462,8 @@ int cmd_describe(int argc, const char **argv, const char 
>> *prefix)
>>  
>>  if (longformat && abbrev == 0)
>>  die(_("--long is incompatible with --abbrev=0"));
>> +if (contains && debug)
>> +die(_("--debug is incompatible with --contains"));
>>  
>>  if (contains) {
>>  struct string_list_item *item;

Re: [PATCH 2/3] git-prompt: add a describe style for any tags

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 15.03.2017 20:25:
> Michael J Gruber <g...@drmicha.warpmail.net> writes:
> 
>> git-prompt has various describe styles, among them "describe" (by
>> annotated tags) and "default" (by exact match with any tag).
>>
>> Add a mode "tag" that describes by any tag, annotated or not.
>>
>> Signed-off-by: Michael J Gruber <g...@drmicha.warpmail.net>
>> ---
>>  contrib/completion/git-prompt.sh | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/contrib/completion/git-prompt.sh 
>> b/contrib/completion/git-prompt.sh
>> index 97eacd7832..c6cbef38c2 100644
>> --- a/contrib/completion/git-prompt.sh
>> +++ b/contrib/completion/git-prompt.sh
>> @@ -82,6 +82,7 @@
>>  # contains  relative to newer annotated tag (v1.6.3.2~35)
>>  # branchrelative to newer tag or branch (master~4)
>>  # describe  relative to older annotated tag (v1.6.3.1-13-gdd42c2f)
>> +# tag   relative to any older tag (v1.6.3.1-13-gdd42c2f)
> 
> I guess this feature makes sense.  
> 
> I just wish we had a well-known unannotated tag we can use for such
> an example; using v1.6.3.1 that is annotated does not help to make
> the distinctin between describe and tag stand out.  We want to
> convey "both annotated one and unannotated one can be used".
> 
> I am wondering if it makes sense to do something like this instead:
> 
> # tag similar to 'describe' but also allow unannotated tags

or inventing a local lightweight tag such as "lastgood-5-gbadbad").
Either way would be fine with me. (I guess it's in next as is now.)

>>  # default   exactly matching tag
>>  #
>>  # If you would like a colored hint about the current dirty state, set
>> @@ -443,6 +444,8 @@ __git_ps1 ()
>>  git describe --contains HEAD ;;
>>  (branch)
>>  git describe --contains --all HEAD ;;
>> +(tag)
>> +git describe --tags HEAD ;;
>>  (describe)
>>  git describe HEAD ;;
>>  (* | default)

[PATCH 2/3] git-prompt: add a describe style for any tags

[Michael J Gruber]

git-prompt has various describe styles, among them "describe" (by
annotated tags) and "default" (by exact match with any tag).

Add a mode "tag" that describes by any tag, annotated or not.

Signed-off-by: Michael J Gruber <g...@drmicha.warpmail.net>
---
 contrib/completion/git-prompt.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 97eacd7832..c6cbef38c2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -82,6 +82,7 @@
 # contains  relative to newer annotated tag (v1.6.3.2~35)
 # branchrelative to newer tag or branch (master~4)
 # describe  relative to older annotated tag (v1.6.3.1-13-gdd42c2f)
+# tag   relative to any older tag (v1.6.3.1-13-gdd42c2f)
 # default   exactly matching tag
 #
 # If you would like a colored hint about the current dirty state, set
@@ -443,6 +444,8 @@ __git_ps1 ()
git describe --contains HEAD ;;
(branch)
git describe --contains --all HEAD ;;
+   (tag)
+   git describe --tags HEAD ;;
(describe)
git describe HEAD ;;
(* | default)
-- 
2.12.0.384.g157040b11f.dirty

[RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs

[Michael J Gruber]

When name-rev constructs possible names for a rev, it assigns the
taggerdate to an annotated tag and ULONG_MAX to other names such as
lightweight tags and branch names. Practically, this rules out even
naming a tagged commit by the tag if that is lightweight and there is
another possible indirect name (e.g. foo~5) coming from an annotated
tag.

Instead, assign the commit date to lightweight tags or branch refs so
that they get their fair chance of being picked up.

Signed-off-by: Michael J Gruber <g...@drmicha.warpmail.net>
---

Originally, I didn't even think of submitting this as is until I noticed that
all tests succeed with it (bar one for git-prompt.h which looked strange
anyways); rather, I thought about another switch "--lightweight" or so.

I still submit it as RFD. This origin of my foray into name-rev came from
git describe, where a fair expectation should be that "--contains" mode
is the same as the odinary mode, albeit with a different direction in
time/traversal. (Technically, describe --contains is name-rev.)

Consider the following:

git init
echo past >tense
git add tense
git commit -m past
git tag -m past -a past
echo present >tense
git commit -am present
git tag present
echo future >tense
git commit -am future

"git describe past present future" gives

past
past-1-g5ad942f
future

because (as documented) it does not consider lightweight tags, and thus
has to describe present from the past.

"git describe --tags past present future" gives

past
present
future

because (as documented) it does consider lightweight tags.

"git describe --contains past present future" gives

past^0
future~1
future^0

and I have a hard time matching that with the documentation
(describe doc claims that --tags is automatic; name-rev doc does not
distinguish between tag types).
"--tags" does not make any difference here, nor does "--all"
(besides fully qualifying the tags).

"git describe --contains past present future" gives

past^0
present
future^0

with this patch (I'm tempted to say: with the present patch),
which is what I would expect.

I'm wondering whether I'm overlooking any side-effects that our test
suite doesn't cover, though. In any case, we may want to have
lightweight tags allowed based on an extra flag (like the
existing --tags for describe, which means something else for name-rev).

 builtin/name-rev.c | 2 ++
 t/t9903-bash-prompt.sh | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 8bdc3eaa6f..75ba7bbad5 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -207,6 +207,8 @@ static int name_ref(const char *path, const struct 
object_id *oid, int flags, vo
if (o && o->type == OBJ_COMMIT) {
struct commit *commit = (struct commit *)o;
 
+   if (taggerdate == ULONG_MAX) /* lightweight tag */
+   taggerdate = commit->date;
path = name_ref_abbrev(path, can_abbreviate_output);
name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref);
}
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 97c9b32c2e..d467d5957d 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -107,7 +107,7 @@ test_expect_success 'prompt - describe detached head - 
contains' '
 '
 
 test_expect_success 'prompt - describe detached head - branch' '
-   printf " ((tags/t2~1))" >expected &&
+   printf " ((b1~1))" >expected &&
git checkout b1^ &&
test_when_finished "git checkout master" &&
(
-- 
2.12.0.384.g157040b11f.dirty

[PATCH 0/3] describe --contains sanity

[Michael J Gruber]

2 patches and 1 RFD around describe (--contains). They are technically
independent, but happened along the same stroll in that area
when I tried to match documentation, my expectations, and reality.

1 and 2 should be no-brainers.

3 is something to ponder for a while.

Michael J Gruber (3):
  describe: debug is incompatible with contains
  git-prompt: add a describe style for any tags
  name-rev: Allow lightweight tags and branch refs

 Documentation/git-describe.txt   | 2 +-
 builtin/describe.c   | 2 ++
 builtin/name-rev.c   | 2 ++
 contrib/completion/git-prompt.sh | 3 +++
 t/t9903-bash-prompt.sh   | 2 +-
 5 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.12.0.384.g157040b11f.dirty

[PATCH 1/3] describe: debug is incompatible with contains

[Michael J Gruber]

`git describe --contains` calls into `git name-rev` which does not have
any searching to do and thus does not display any debug information.

Say so in the documentation and catch the incompatible arguments.

Signed-off-by: Michael J Gruber <g...@drmicha.warpmail.net>
---
 Documentation/git-describe.txt | 2 +-
 builtin/describe.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index 8755f3af7b..0f9adb6e9a 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -69,7 +69,7 @@ OPTIONS
 --debug::
Verbosely display information about the searching strategy
being employed to standard error.  The tag name will still
-   be printed to standard out.
+   be printed to standard out. This is incompatible with --contains.
 
 --long::
Always output the long format (the tag, the number of commits
diff --git a/builtin/describe.c b/builtin/describe.c
index 76c18059bf..01a6d159a0 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -462,6 +462,8 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
 
if (longformat && abbrev == 0)
die(_("--long is incompatible with --abbrev=0"));
+   if (contains && debug)
+   die(_("--debug is incompatible with --contains"));
 
if (contains) {
struct string_list_item *item;
-- 
2.12.0.384.g157040b11f.dirty

[PATCH] git-status: make porcelain more robust

[Michael J Gruber]

git status provides a porcelain mode for porcelain writers with a
supposedly stable (plumbing) interface.
7a76c28ff2 ("status: disable translation when --porcelain is used", 2014-03-20)
made sure that ahead/behind info is not translated (i.e. is stable).

Make sure that the remaining two strings (initial commit, detached head)
are stable, too.

These changes are for the v1 porcelain interface. While we do have a perfectly
stable v2 porcelain interface now, some tools (such as
powerline-gitstatus) are written against v1 and profit from fixing v1
without any changes on their side.

Signed-off-by: Michael J Gruber <g...@drmicha.warpmail.net>
---
 wt-status.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index d47012048f..234e77a6d6 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1730,12 +1730,14 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
return;
branch_name = s->branch;
 
+#define LABEL(string) (s->no_gettext ? (string) : _(string))
+
if (s->is_initial)
-   color_fprintf(s->fp, header_color, _("Initial commit on "));
+   color_fprintf(s->fp, header_color, LABEL(N_("Initial commit on 
")));
 
if (!strcmp(s->branch, "HEAD")) {
color_fprintf(s->fp, color(WT_STATUS_NOBRANCH, s), "%s",
- _("HEAD (no branch)"));
+ LABEL(N_("HEAD (no branch)")));
goto conclude;
}
 
@@ -1760,8 +1762,6 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
if (!upstream_is_gone && !num_ours && !num_theirs)
goto conclude;
 
-#define LABEL(string) (s->no_gettext ? (string) : _(string))
-
color_fprintf(s->fp, header_color, " [");
if (upstream_is_gone) {
color_fprintf(s->fp, header_color, LABEL(N_("gone")));
-- 
2.12.0.384.g157040b11f.dirty

Re: Stable GnuPG interface, git should use GPGME

[Michael J Gruber]

Bernhard E. Reiter venit, vidit, dixit 13.03.2017 13:49:
> Am Montag 13 März 2017 11:14:57 schrieb Michael J Gruber:
>> Ævar Arnfjörð Bjarmason venit, vidit, dixit 10.03.2017 15:23:
>>> On Fri, Mar 10, 2017 at 11:00 AM, Bernhard E. Reiter
> 
>>>> please consider using libgpgme interfacing to GnuPG, because the gpg
>>>> command-line interface is not considered an official API to GnuPG by the
>>>> GnuPG-devs and thus potentially unstable.
> 
> [example of gpg2 vs gpg option incompatibility cut]
> 
>>> Using the library sounds good, but a shorter-term immediate fix would
>>> be to figure out what bug you encountered in our use of the
>>> command-line version, and see if we've fixed that already or not.
> 
>> As far as I know, Git handles different GPG versions just fine.
> 
> As mentioned before: explicitely setting gpg.program to gpg2 helps if gpg
> chokes on the new config. Trying the `gpg2` binary first can be a simple fix. 
> Using libgpgme potentially solves this and other compatility options.
> 
>> The problem is the "difficult" upgrade path and mixed installations with
>> gpg and gpg2.1+ that some distributions force upon you:
>>
>> As soon as you start gpg2.1, your (secret) key store is migrated to a
>> new format without technically invalidating it. Similarly, users may
>> enter gpg2.1+-only comand in the config that is actually shared with
>> gpg, throwing off any use of gpg - not just by git, but also by anything
>> that your distro requires gpg for (such as packaging tools and the like).
> 
> Yes, this is another example why trying `gpg2? first by default or using 
> libgpgme keeps trouble away from users.

No, not at all. On the contrary: Using gpg2.1 without being asked to by
the user will migrate his key store! This can lead to tremendous
problems when a user manages his secret key store using gpg and git uses
the other secret key store (via gpg2.1)!

>> In short: Users will run into problems anyway; git provides the quick
>> way out (git config gpg.program gpg2), users won't be as lucky with
>> other things that require gpg.
> 
> Application using libgpgme will behave fine and many user facing components 
> use it already. 
> 
>> As for the library: While - technically speaking - the command line is
>> not a stable API for gpg, it does work across versions of gpg, and gpg
> 
> ... to some extend.

I have no idea what you're implying here - but I have a pretty good idea
of what works in current git and what not, including gpg usage (which is
the qualifier that you cut out).

>> 2.2 will be the first real stable branch that uses the new key store
>> layout. So I'd rather wait for that to stabilize before going away from
>> what turned out to be most stable so far.
> 
> It is not just about the key-store change as mentioned before. However
> I agree that a potential switch should be done with a current version of 
> gpgme 
> that already has support for GnuPG 2.1/2, e.g. gpgme v1.8.0.

Unfortunately, gpgme does not solve the interoperability problems
between gpg (1, classic, stable maint mode) or gpg2.0 (stable) and
gpg2.1 (modern) key stores, and gpg2.2 (modern, stable) is not released yet.

>> Note that we (git) refrain from parsing ordinary output/return codes of
>> gpg and use status-fd as we should (and as documented).
> 
> It is good to use --status-fd and --with-colons when calling gpg, you still 
> have to parse the results of status-fd as described in doc/DETAILs. 
> https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;hb=HEAD

Again, have you checked what we are doing in git land?

Your agenda is pretty clear. It's also pretty clear from the above that
gpgme is not the solution to the problem which is introduced by the
migration path to versions of gpg which are not declared stable by the
gpg project, away from a gpg version which is not obsoleted by the gpg
project (2.0, maybe 1).

And, really, key store migration was the only "major" thing we had to
think about to cope with gpg 2.1+ - it affected the test suite setup.

So, once 2.2 is out and stable and mainstream and we don't risk
migrating users' secret key store sneakily I'm all for defaulting to
gpg2, and then is a good time for us to look into gpgme.

Michael

Re: Stable GnuPG interface, git should use GPGME

[Michael J Gruber]

Ævar Arnfjörð Bjarmason venit, vidit, dixit 10.03.2017 15:23:
> On Fri, Mar 10, 2017 at 11:00 AM, Bernhard E. Reiter
>  wrote:
>> Dear Git-Devs,
> 
> I haven't contributed to Git's GPG code, but I'm taking the liberty of
> CC-ing some people who have.
> 
>> git uses an pipe-and-exec approach to running a GnuPG binary
>> as writen in the documentation [1]:
>>
>> gpg.program
>>Use this custom program instead of "gpg" found on $PATH when 
>> making
>>or verifying a PGP signature. The program must support the same
>>command-line interface as GPG
>>
>> please consider using libgpgme interfacing to GnuPG, because the gpg
>> command-line interface is not considered an official API to GnuPG by the
>> GnuPG-devs and thus potentially unstable.
>>
>> == Details
>>
>> I'm involved in GnuPG development. For most applications using libgpgme is 
>> the
>> way what GnuPG-devs would recommend, also see
>>
>>   https://wiki.gnupg.org/APIs .
>>
>> GnuPG devs are making a good effort of trying to keep the command-line
>> interface stable, though it is not for sure. Git is only using a small part
>> of the interface, so the risk when keeping the current way is small.
>> Still I believe git's stability and usability would profit when moving to
>> libgpgme, especially with the coming move to GnuPG 2.2, better diagnosing
>> messages and for cross-plattform usage.
>>
>> == Usability problem with `gpg2` vs `gpg`
>>
>> My use case today was signing and git by default found the `gpg` binary by
>> default and the command failed. The reason is that I have `gpg2` installed
>> and most applications use it right away. So git failed signing because
>> the .gnupg configuration of the user was not ready for the old `gpg` which is
>> still installed on Debian GNU/Linux for purposes of the operating system. If
>> git would have used libgpgme, gpgme would have choosen the most uptodate
>> version of `gpg` available (or configured) without me intervening via
>> gpg.program. Now because of this problem you could adding a check for `gpg2`
>> and fallback to `gpg`, but even better would be to move to libgpgme. >:)
> 
> I'm on Debian but haven't had these issues. What's your gpg & gpg2
> --version & Debian release? And what in particular failed?
> 
> And what git version was this? I see we've had a couple of workarounds
> for gpg2, in particular Linus's v2.8.4-1-gb624a3e67f, but if you have
> v2.10.0 or later that won't fix whatever issue you had.
> 
> Using the library sounds good, but a shorter-term immediate fix would
> be to figure out what bug you encountered in our use of the
> command-line version, and see if we've fixed that already or not.
> Regardless of what we do with a gpg library in the future some distros
> might want to backport such a small patch if we can come up with it.

As far as I know, Git handles different GPG versions just fine.

The problem is the "difficult" upgrade path and mixed installations with
gpg and gpg2.1+ that some distributions force upon you:

As soon as you start gpg2.1, your (secret) key store is migrated to a
new format without technically invalidating it. Similarly, users may
enter gpg2.1+-only comand in the config that is actually shared with
gpg, throwing off any use of gpg - not just by git, but also by anything
that your distro requires gpg for (such as packaging tools and the like).

In short: Users will run into problems anyway; git provides the quick
way out (git config gpg.program gpg2), users won't be as lucky with
other things that require gpg.

As for the library: While - technically speaking - the command line is
not a stable API for gpg, it does work across versions of gpg, and gpg
2.2 will be the first real stable branch that uses the new key store
layout. So I'd rather wait for that to stabilize before going away from
what turned out to be most stable so far.

Note that we (git) refrain from parsing ordinary output/return codes of
gpg and use status-fd as we should (and as documented).

Michael

Re: show all merge conflicts

[Michael J Gruber]

G. Sylvie Davies venit, vidit, dixit 29.01.2017 07:45:
> On Sat, Jan 28, 2017 at 6:28 AM, Jeff King  wrote:
>> On Fri, Jan 27, 2017 at 09:42:41PM -0800, G. Sylvie Davies wrote:
>>
>>> Aside from the usual "git log -cc", I think this should work (replace
>>> HEAD with whichever commit you are analyzing):
>>>
>>> git diff --name-only HEAD^2...HEAD^1 > m1
>>> git diff --name-only HEAD^1...HEAD^2 > b1
>>> git diff --name-only HEAD^1..HEAD> m2
>>> git diff --name-only HEAD^2..HEAD> b2
>>>
>>> If files listed between m1 and b2 differ, then the merge is dirty.
>>> Similarly for m2 and b1.
>>>
>>> More information here:
>>>
>>> http://stackoverflow.com/questions/27683077/how-do-you-detect-an-evil-merge-in-git/41356308#41356308
>>
>> I don't think that can reliably find evil merges, since it looks at the
>> file level. If you had one hunk resolved for "theirs" and one hunk for
>> "ours" in a given file, then the file will be listed in each diff,
>> whether it has evil hunks or not.
>>
> 
> Well, you have to do both.  Do "git show -c" to catch that one
> ("theirs" for one hunk, "ours" for the other, same file).
> 
> And then do that sequence of the 4 "git diff" commands to identify
> dirty merges where "theirs" or "ours" was applied to entire files, and
> thus not showing up in the "git show -c".
> 
>> I don't think this is just about evil merges, though. For instance,
>> try:
>>
>>   seq 1 10 >file
>>   git add file
>>   git commit -m base
>>
>>   sed s/4/master/ tmp && mv tmp file
>>   git commit -am master
>>
>>   git checkout -b other HEAD^
>>   sed s/4/other/ tmp && mv tmp file
>>   git commit -am other
>>
>>   git merge master
>>   git checkout --ours file
>>   git commit -am merged
>>
>>   merge=$(git rev-parse HEAD)
>>
>> The question is: were there conflicts in $merge, and how were they
>> resolved?
>>
>> That isn't an evil merge, but there's still something interesting to
>> show that "git log --cc" won't display.
>>
>> Replaying the merge like:
>>
>>   git checkout $merge^1
>>   git merge $merge^2
>>   git diff -R $merge
>>
>> shows you the patch to go from the conflict state to the final one.
>>
> 
> I know the stackoverflow question asks "how to detect evil merges",
> and I go along with that in my answer.  But honestly I prefer to call
> them dirty rather than evil, and by "dirty" I just mean merges that
> did not resolve cleanly via "git merge", and had some form of user
> intervention, be it conflict resolution, or other strange things.
> 
> The trick I propose with the sequence of 4 "git diff" commands
> identifies that merge from your example as dirty:
> 
> $ cat b1 m2
> file
> 
> $ cat b2 m1
> file
> file
> 
> The trick doesn't really tell you much except that the merge is dirty.
> If you notice that the "m2" file is empty, I think that's one way to
> realize that master's edit was dropped, and therefore "other" won.
> 
> Maybe it even merged cleanly but someone did a "git commit --amend" to
> make it the merge dirty after the fact.
> 
> I do like your approach, it's very simple and reliable.  But in my
> situation I'm writing pre-receive hooks for bare repos, so I don't
> think I can actually do "git merge"!
> 
> I think my suggestion would work for OP, as long as they also run "git
> show -c" alongside it.   (And your suggestion would work, too, of
> course).

If you're curious, I kept rebasing Thomas' remerge-diff (on top of our
next) so far. You can find it at

https://github.com/mjg/git/tree/remerge-diff

if you're interested. I don't know what problems were found back then,
or what it would take to get this in-tree now.

Michael