Speed of git branch --contains

[Andreas Krey]

Hi everybody,

I'm just looking at some scripts that do a 'git branch --contains $id --remote'
for each new commit in a repo, and unfortunately each invokation already
takes four minutes.

It feels like git branch does the reachability detection separately
for each branch potentially listed. The alternative would be to

- invert the parent map to a child map,
- use that to compute the set of commits that contain $id,
- then use that as predicate whether to show a given branch
  (show iff its head is in the set)

That would speed things up considerably,
but what are the chances to see that change in git?

I can do that as well within the script, with the additional
benefit that I only need to do the inversion once, but I might
instead take a stab at git branch.

- Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds 
Date: Fri, 22 Jan 2010 07:29:21 -0800

Problem with git and proxy server with authentication

[Aleksey Yaroslavcev]

Hi.
(sory for my english - it's not native for me)

I use proxy-server with authentication.
I have two instances of PortableGit: version 2.10.0.windows.1 and
2.16.1.windows.1
I added proxy settings in ~/.gitconfig.

If I try to pull my repository with version 2.16.1.windows.1 , I get
error message:
"fatal: unable to access
'https://aleksey_yaroslav...@bitbucket.org/aleksey_yaroslavcev/testgitflow.git/':
Proxy CONNECT aborted"

But pulling with version 2.10.0.windows.1 working fine with the same
settings in ~/.gitconfig.

I decided to figure out what the problem is and found out the
following. There is a difference in the way how git making connection
through proxy. See the attached screenshots from WireShark.

In old version:
1. creating connection to proxy
2. trying to connect without authentication
3. proxy saying "407 Authentication Required" and requesting
termination of connection(FIN flag in TCP packet)
4. reseting conecction(RST flag in TCP packet)
5. creating another connection
6. trying to connect with authentication
7. Bingo!

In new version:
1. creating connection to proxy
2. trying to connect without authentication
3. proxy saying "407 Authentication Required" and requesting
termination of connection (FIN flag in TCP packet)
4. trying to connect with authentication in the same connection but
proxy is resetting connection (RST flag in TCP packet)

Re: [PATCH v6] mru: Replace mru.[ch] with list.h implementation

[Jeff King]

On Tue, Jan 23, 2018 at 06:46:51PM -0500, Gargi Sharma wrote:

> Replace the custom calls to mru.[ch] with calls to list.h. This patch is
> the final step in removing the mru API completely and inlining the logic.
> This patch leads to significant code reduction and the mru API hence, is
> not a useful abstraction anymore.
> 
> Signed-off-by: Gargi Sharma 

Thanks, this version looks good to me.

-Peff

Re: [PATCH] mailinfo: avoid segfault when can't open files

[Jeff King]

On Tue, Jan 23, 2018 at 11:54:17PM -0300, Juan F. Codagnone wrote:

> If  or  files can't be opened, clear_mailinfo crash as
> it follows NULL pointers.
> 
> Can be reproduced using `git mailinfo . .`

Thanks for finding this.

Looking at the offending code and your solution, it looks like:

  1. mailinfo() sometimes allocates mi->p_hdr_data and mi->s_hdr_data
 and sometimes not, depending on how far we get before seeing an
 error. The caller cannot know whether we did so or not based on
 seeing an error return, but most call clear_mailinfo() if it wants
 to avoid a leak.

  2. There are two callers of mailinfo(). git-am simply dies on an
 error, and so is unaffected. But git-mailinfo unconditionally calls
 clear_mailinfo() before returning, regardless of the return code.

  3. When we get to clear_mailinfo(), the arrays are either populated or
 are NULL. We know they're initialized to NULL because of
 setup_mailinfo(), which zeroes the whole struct.

So I think your fix does the right thing. I do think this is a pretty
awkward interface, and it would be less error-prone if either[1]:

  a. we bumped the allocation of these arrays up in mailinfo() so
 that they were simply always initialized. This fixes the bug in
 clear_mailinfo(), but also any other function which looks at the
 mailinfo struct (though I don't think there are any such cases).

  b. we had mailinfo() clean up after itself on error, so that it was
 always in a de-initialized state.

But given the lack of callers, it may not be worth the effort. So I'm OK
with this solution. It may be worth giving an abbreviated version of the
above explanation in the commit message. Perhaps:

  If  or  files can't be opened, then mailinfo() returns an
  error before it even initializes mi->p_hdr_data or mi->s_hdr_data.
  When cmd_mailinfo() then calls clear_mailinfo(), we dereference the
  NULL pointers trying to free their contents.

As for the patch itself, it looks correct but I saw two style nits:

> diff --git a/mailinfo.c b/mailinfo.c
> index a89db22ab..035abbbf5 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -1167,11 +1167,13 @@ void clear_mailinfo(struct mailinfo *mi)
>   strbuf_release(>inbody_header_accum);
>   free(mi->message_id);
>  
> - for (i = 0; mi->p_hdr_data[i]; i++)
> - strbuf_release(mi->p_hdr_data[i]);
> + if(mi->p_hdr_data != NULL)
> + for (i = 0; mi->p_hdr_data[i]; i++)
> + strbuf_release(mi->p_hdr_data[i]);

We usually say "if (" with an extra space. And we generally just check
pointers for their truth value. So:

  if (mi->p_hdr_data) {
for (i = 0; ...)

-Peff

[1] Actually, it seems a little funny that we use xcalloc() here at all,
since the size is determined by a compile-time constant. Why not
just put an array directly into the struct and let it get zeroed
with the rest of the struct? That sidesteps the question of whether
we need to clear() after an error return, but it would fix this bug. :)

[PATCH] mailinfo: avoid segfault when can't open files

[Juan F. Codagnone]

If  or  files can't be opened, clear_mailinfo crash as
it follows NULL pointers.

Can be reproduced using `git mailinfo . .`

Signed-off-by: Juan F. Codagnone 
---
 mailinfo.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index a89db22ab..035abbbf5 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -1167,11 +1167,13 @@ void clear_mailinfo(struct mailinfo *mi)
strbuf_release(>inbody_header_accum);
free(mi->message_id);
 
-   for (i = 0; mi->p_hdr_data[i]; i++)
-   strbuf_release(mi->p_hdr_data[i]);
+   if(mi->p_hdr_data != NULL)
+   for (i = 0; mi->p_hdr_data[i]; i++)
+   strbuf_release(mi->p_hdr_data[i]);
free(mi->p_hdr_data);
-   for (i = 0; mi->s_hdr_data[i]; i++)
-   strbuf_release(mi->s_hdr_data[i]);
+   if(mi->s_hdr_data != NULL)
+   for (i = 0; mi->s_hdr_data[i]; i++)
+   strbuf_release(mi->s_hdr_data[i]);
free(mi->s_hdr_data);
 
while (mi->content < mi->content_top) {
-- 
2.14.3

Your long awaited part payment of $2.5.000.00Usd

[UNITED NATIONS]

Attention: Beneficiary,

Your long awaited part payment of $2.5.000.00Usd (TWO MILLION FIVE
Hundred Thousand United State Dollars)  is ready for immediate release
to you, and it was electronically credited into an ATM Visa Card for
easy delivery.

Your new Payment Reference No.- 6363836,
Password No: 006786,
Pin Code No: 1787
Your Certificate of Merit Payment No: 05872,
Released Code No: 1134;
Immediate Telex confirmation No:- 043612;
Secret Code No: TBKTA28

Re-Confirm;
Your Names: |Address: |Cell Phone: |Fax: | Amount: |
Your immediate response is needed
UBA Bank

Person to Contact:MR KELLY HALL  the Director of the International Audit
unit ATM Payment Center,

Email: ubabf_uba...@financier.com
TELEPHONE: +226 68251393

Note: Your file will expire after 14 days if there is no response, and then
we will not have any option than to return your fund to IMF as unclaimed.
Your swift response will enhance our service, thanks for your co-operation;

Regards.
Mr JOHN MARK.

Re: [PATCH 5/5] travis-ci: don't fail if user already exists on 32 bit Linux build job

[Duy Nguyen]

On Tue, Jan 23, 2018 at 11:46 PM, Jeff King  wrote:
>> The build job on Travis CI always starts with a fresh Docker
>> container, so this change doesn't make a difference there.
>
> I wonder if that is fixable. Installing dependencies into the container
> takes quite a lot of time, and is just wasted effort.

I agree (because apt-get install phase took forever on my laptop).
I've seen people just include a Dockerfile that builds everything in
(except things like creating user matching host). First try to fetch
it from docker hub (or a local registry but I don't think it applies
to us). If not found, rebuild the image with Dockerfile and run a new
container with it. Every time somebody changes dockerfile, they are
supposed to step the image version up.

Of course this is still wasted effort unless somebody pushes images to
docker registry and the script has to rebuild the image every single
time [1]. Somebody could do that manually. I see docker hub has some
automated rebuild feature, perhaps that'll work for us.

[1] It still benefits users who run ci scripts manually because the
created image will stay in their local machine.
-- 
Duy

[PATCH v6] mru: Replace mru.[ch] with list.h implementation

[Gargi Sharma]

Replace the custom calls to mru.[ch] with calls to list.h. This patch is
the final step in removing the mru API completely and inlining the logic.
This patch leads to significant code reduction and the mru API hence, is
not a useful abstraction anymore.

Signed-off-by: Gargi Sharma 
---
The commit has been built on top of ot/mru-on-list branch.

Changes in v6:
- Replace mru_clear with INIT_LIST_HEAD() to reset
  the mru list.
Changes in v5:
- Remove list_move_to_front() as it is redundant.
Changes in v4:
- Fixing minor style issues.
Changes in v3:
- Make the commit message more descriptive.
- Remove braces after if statement.
Changes in v2:
- Add a move to front function to the list API.
- Remove memory leak.
- Remove redundant remove operations on the list.

A future improvement could be removing/changing the
type of nect pointer or dropping it entirely. See discussion
here:
https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/
---
---
 Makefile   |  1 -
 builtin/pack-objects.c |  9 -
 cache.h|  8 
 mru.c  | 27 ---
 mru.h  | 40 
 packfile.c | 17 +
 sha1_file.c|  1 -
 7 files changed, 17 insertions(+), 86 deletions(-)
 delete mode 100644 mru.c
 delete mode 100644 mru.h

diff --git a/Makefile b/Makefile
index ed4ca43..4a79ec5 100644
--- a/Makefile
+++ b/Makefile
@@ -814,7 +814,6 @@ LIB_OBJS += merge.o
 LIB_OBJS += merge-blobs.o
 LIB_OBJS += merge-recursive.o
 LIB_OBJS += mergesort.o
-LIB_OBJS += mru.o
 LIB_OBJS += name-hash.o
 LIB_OBJS += notes.o
 LIB_OBJS += notes-cache.o
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ba81234..6a0b8e8 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -24,7 +24,7 @@
 #include "reachable.h"
 #include "sha1-array.h"
 #include "argv-array.h"
-#include "mru.h"
+#include "list.h"
 #include "packfile.h"
 
 static const char *pack_usage[] = {
@@ -1012,9 +1012,8 @@ static int want_object_in_pack(const unsigned char *sha1,
return want;
}
 
-   list_for_each(pos, _git_mru.list) {
-   struct mru *entry = list_entry(pos, struct mru, list);
-   struct packed_git *p = entry->item;
+   list_for_each(pos, _git_mru) {
+   struct packed_git *p = list_entry(pos, struct packed_git, mru);
off_t offset;
 
if (p == *found_pack)
@@ -1031,7 +1030,7 @@ static int want_object_in_pack(const unsigned char *sha1,
}
want = want_found_object(exclude, p);
if (!exclude && want > 0)
-   mru_mark(_git_mru, entry);
+   list_move(>mru, _git_mru);
if (want != -1)
return want;
}
diff --git a/cache.h b/cache.h
index 49b083e..cc09e3b 100644
--- a/cache.h
+++ b/cache.h
@@ -4,7 +4,7 @@
 #include "git-compat-util.h"
 #include "strbuf.h"
 #include "hashmap.h"
-#include "mru.h"
+#include "list.h"
 #include "advice.h"
 #include "gettext.h"
 #include "convert.h"
@@ -1566,6 +1566,7 @@ struct pack_window {
 
 extern struct packed_git {
struct packed_git *next;
+   struct list_head mru;
struct pack_window *windows;
off_t pack_size;
const void *index_data;
@@ -1587,10 +1588,9 @@ extern struct packed_git {
 } *packed_git;
 
 /*
- * A most-recently-used ordered version of the packed_git list, which can
- * be iterated instead of packed_git (and marked via mru_mark).
+ * A most-recently-used ordered version of the packed_git list.
  */
-extern struct mru packed_git_mru;
+extern struct list_head packed_git_mru;
 
 struct pack_entry {
off_t offset;
diff --git a/mru.c b/mru.c
deleted file mode 100644
index 8f3f34c..000
--- a/mru.c
+++ /dev/null
@@ -1,27 +0,0 @@
-#include "cache.h"
-#include "mru.h"
-
-void mru_append(struct mru *head, void *item)
-{
-   struct mru *cur = xmalloc(sizeof(*cur));
-   cur->item = item;
-   list_add_tail(>list, >list);
-}
-
-void mru_mark(struct mru *head, struct mru *entry)
-{
-   /* To mark means to put at the front of the list. */
-   list_del(>list);
-   list_add(>list, >list);
-}
-
-void mru_clear(struct mru *head)
-{
-   struct list_head *pos;
-   struct list_head *tmp;
-
-   list_for_each_safe(pos, tmp, >list) {
-   free(list_entry(pos, struct mru, list));
-   }
-   INIT_LIST_HEAD(>list);
-}
diff --git a/mru.h b/mru.h
deleted file mode 100644
index 80a589e..000
--- a/mru.h
+++ /dev/null
@@ -1,40 +0,0 @@
-#ifndef MRU_H
-#define MRU_H
-
-#include "list.h"
-
-/**
- * A simple most-recently-used cache, backed by a doubly-linked list.
- *
- * 

What's cooking in git.git (Jan 2018, #03; Tue, 23)

[Junio C Hamano]

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

Many topics in 'next' have been merged to 'master' while others are
tentatively kicked back to 'pu', to give them a chance to be rerolled
if the authors of them wish to do so.

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

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

--
[Graduated to "master"]

* ab/commit-m-with-fixup (2017-12-22) 2 commits
  (merged to 'next' on 2018-01-11 at 41255c464f)
 + commit: add support for --fixup  -m""
 + commit doc: document that -c, -C, -F and --fixup with -m error

 "git commit --fixup" did not allow "-m" option to be used
 at the same time; allow it to annotate resulting commit with more
 text.


* ab/doc-cat-file-e-still-shows-errors (2018-01-10) 1 commit
  (merged to 'next' on 2018-01-12 at 080bb1d397)
 + cat-file doc: document that -e will return some output

 Doc update.


* ab/perf-grep-threads (2018-01-04) 1 commit
  (merged to 'next' on 2018-01-09 at 91889574fb)
 + perf: amend the grep tests to test grep.threads

 More perf tests for threaded grep


* as/read-tree-prefix-doc-fix (2018-01-09) 1 commit
  (merged to 'next' on 2018-01-12 at 895c72e5c3)
 + doc/read-tree: remove obsolete remark

 Doc update.


* bw/oidmap-autoinit (2017-12-27) 1 commit
  (merged to 'next' on 2018-01-11 at f941e013b4)
 + oidmap: ensure map is initialized

 Code clean-up.


* cc/codespeed (2018-01-05) 7 commits
  (merged to 'next' on 2018-01-09 at 8578089a2b)
 + perf/run: read GIT_PERF_REPO_NAME from perf.repoName
 + perf/run: learn to send output to codespeed server
 + perf/run: learn about perf.codespeedOutput
 + perf/run: add conf_opts argument to get_var_from_env_or_config()
 + perf/aggregate: implement codespeed JSON output
 + perf/aggregate: refactor printing results
 + perf/aggregate: fix checking ENV{GIT_PERF_SUBSECTION}

 "perf" test output can be sent to codespeed server.


* dk/describe-all-output-fix (2017-12-27) 1 commit
  (merged to 'next' on 2017-12-28 at c6254494e3)
 + describe: prepend "tags/" when describing tags with embedded name

 An old regression in "git describe --all $annotated_tag^0" has been
 fixed.


* ew/empty-merge-with-dirty-index (2018-01-09) 1 commit
  (merged to 'next' on 2018-01-09 at 6bcda11248)
 + Merge branch 'ew/empty-merge-with-dirty-index-maint' into 
ew/empty-merge-with-dirty-index
 (this branch uses ew/empty-merge-with-dirty-index-maint.)

 "git merge -s recursive" did not correctly abort when the index is
 dirty, if the merged tree happened to be the same as the current
 HEAD, which has been fixed.


* jc/merge-symlink-ours-theirs (2018-01-03) 1 commit
  (merged to 'next' on 2018-01-05 at 63ebfc45eb)
 + merge: teach -Xours/-Xtheirs to symbolic link merge

 "git merge -Xours/-Xtheirs" learned to use our/their version when
 resolving a conflicting updates to a symbolic link.


* jh/object-filtering (2018-01-08) 1 commit
  (merged to 'next' on 2018-01-11 at 56808f6969)
 + oidset: don't return value from oidset_init

 Hotfix for a topic already in 'master'.


* jk/abort-clone-with-existing-dest (2018-01-03) 4 commits
  (merged to 'next' on 2018-01-09 at 3c8e83c3a7)
 + clone: do not clean up directories we didn't create
 + clone: factor out dir_exists() helper
 + t5600: modernize style
 + t5600: fix outdated comment about unborn HEAD

 "git clone $there $here" is allowed even when here directory exists
 as long as it is an empty directory, but the command incorrectly
 removed it upon a failure of the operation.


* jm/svn-pushmergeinfo-fix (2017-09-17) 1 commit
  (merged to 'next' on 2018-01-05 at 6cb237ea44)
 + git-svn: fix svn.pushmergeinfo handling of svn+ssh usernames.

 "git svn dcommit" did not take into account the fact that a
 svn+ssh:// URL with a username@ (typically used for pushing) refers
 to the same SVN repository without the username@ and failed when
 svn.pushmergeinfo option is set.


* js/fix-merge-arg-quoting-in-rebase-p (2018-01-05) 1 commit
  (merged to 'next' on 2018-01-09 at 91f5601e9c)
 + rebase -p: fix quoting when calling `git merge`

 "git rebase -p -X" did not propagate the option properly
 down to underlying merge strategy backend.


* js/test-with-ws-in-path (2018-01-10) 1 commit
  (merged to 'next' on 2018-01-10 at c44db26fe4)
 + t3900: add some more quotes

 Hot fix to a test.


* ma/bisect-leakfix (2018-01-03) 1 commit
  (merged to 'next' on 2018-01-09 at 2ef8b59d1b)
 + bisect: fix a regression causing a segfault

 A hotfix for a recent update that broke 'git bisect'.


* mm/send-email-fallback-to-local-mail-address (2018-01-08) 3 commits
  (merged to 'next' on 2018-01-17 at dd4867706b)
 + send-email: add test for Linux's get_maintainer.pl
 

[PATCH v3 04/11] fetch tests: re-arrange arguments for future readability

[Ævar Arnfjörð Bjarmason]

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

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

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

[PATCH v3 08/11] git remote doc: correct dangerous lies about what prune does

[Ævar Arnfjörð Bjarmason]

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

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

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

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

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

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

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

[PATCH v3 00/11] document & test fetch pruning & add fetch.pruneTags

[Ævar Arnfjörð Bjarmason]

Addresses Junio's comments on v2 + more fixes. The 

Ævar Arnfjörð Bjarmason (11):
  fetch: don't redundantly NULL something calloc() gave us
  fetch: stop accessing "remote" variable indirectly

Moved these patches to the beginning. No changes except a note to the
commit message saying how these trivial changes relate (or not) to
subsequent changes.

  fetch tests: refactor in preparation for testing tag pruning

No changes.

  fetch tests: re-arrange arguments for future readability

s/arrange/re-arrange/ in commit message subject.

  fetch tests: add a tag to be deleted to the pruning tests

No changes.

  fetch tests: test --prune and refspec interaction

I'm now just skipping quoting things like +refs/... on the
command-line, which as grepping the rest of the test suite shows is
fine, this eliminated the need for "fetch tests: double quote a
variable for interpolation" so I've ejected it.

  git fetch doc: add a new section to explain the ins & outs of pruning

No changes.

  git remote doc: correct dangerous lies about what prune does

Rewording of doc per Junio's suggestion.


  git-fetch & config doc: link to the new PRUNING section

No changes.

  fetch tests: add scaffolding for the new fetch.pruneTags

No changes except continuing remove refspec quoting changes noted
above & using +refs/tags/... instead of refs/tags/... for consistency
with the last patch...

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

We now consistently use a + prefixed refspec for tag pruning, even
though it yields the same result. See the discussion in
87tvvdh2vh@evledraar.gmail.com, I think it's less confusing.

Fix regex in --replace-all command in the commit message.

Rewording to address Junio's comments.

The short help for --prune-tags is now:

-   N_("prune local tags not found on remote")),
+   N_("prune local tags no longer on remote and clobber changed 
tags")),

Add --prune-tags to the bash completion.

 Documentation/config.txt   |  20 -
 Documentation/fetch-options.txt|  18 +++-
 Documentation/git-fetch.txt|  76 +
 Documentation/git-remote.txt   |  14 +--
 builtin/fetch.c|  37 +++-
 contrib/completion/git-completion.bash |   2 +-
 remote.c   |   5 +-
 remote.h   |   3 +
 t/t5510-fetch.sh   | 152 -
 9 files changed, 275 insertions(+), 52 deletions(-)

-- 
2.15.1.424.g9478a66081

[PATCH v3 01/11] fetch: don't redundantly NULL something calloc() gave us

[Ævar Arnfjörð Bjarmason]

Stop redundantly NULL-ing the last element of the refs structure,
which was retrieved via calloc(), and is thus guaranteed to be
pre-NULL'd.

This code dates back to b888d61c83 ("Make fetch a builtin",
2007-09-10), where wasn't any reason to do this back then either, it's
just something left over from when git-fetch was initially introduced.

The initial motivation for this change was to make a subsequent change
which would also modify the refs variable smaller, since it won't have
to copy this redundant "NULL the last + 1 item" pattern.

As it turns out that change was bad, and better done differently, but
as this pattern is still pointless let's fix it.

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

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7bbcd26faf..b34665db9e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1302,7 +1302,6 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv)
} else
refs[j++] = argv[i];
}
-   refs[j] = NULL;
ref_nr = j;
}
 
-- 
2.15.1.424.g9478a66081

[PATCH v3 06/11] fetch tests: test --prune and refspec interaction

[Ævar Arnfjörð Bjarmason]

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

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

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

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

[PATCH v3 11/11] fetch: add a --fetch-prune option and fetch.pruneTags config

[Ævar Arnfjörð Bjarmason]

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

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

Or simply:

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

Instead of the much more verbose:

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

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

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

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

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

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

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

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

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

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0f27af5760..ae86455876 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1401,6 +1401,14 @@ fetch.prune::
option was given on the command line.  See also `remote..prune`
and the PRUNING section of linkgit:git-fetch[1].
 
+fetch.pruneTags::
+   If true, fetch will automatically behave as if the
+   `refs/tags/*:refs/tags/*` refspec was provided when pruning,
+   if not set already. This allows for setting both this option
+   and `fetch.prune` to maintain a 1=1 mapping to upstrem
+   refs. See also `remote..pruneTags` and the PRUNING
+   section of linkgit:git-fetch[1].
+
 fetch.output::
Control how ref update status is printed. Valid values are
`full` and `compact`. Default value is `full`. See section
@@ -2945,6 +2953,12 @@ remote..prune::
remove any remote-tracking references that no longer exist on the
remote (as if the `--prune` option was given on the command line).
Overrides `fetch.prune` settings, if any.
+
+remote..pruneTags::
+   When set to true, fetching from this remote by default will also
+   remove any local tags that no longer exist on the remote if pruning
+   is activated in general via `remote..prune`, `fetch.prune` or
+   `--prune`. Overrides `fetch.pruneTags` settings, if any.
 +
 See also `remote..prune` and the PRUNING section of
 linkgit:git-fetch[1].
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 9f5c85ad96..dc13bed741 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -73,7 +73,20 @@ ifndef::git-pull[]
are fetched due to an explicit refspec (either on the command
line or in the remote configuration, for example if the remote
was cloned with the --mirror option), then they are also
-   subject to pruning.
+   subject to pruning. Supplying `--prune-tags` is a shorthand for
+   providing the tag refspec.
++
+See the PRUNING section below for more details.
+
+-P::
+--prune-tags::
+   Before fetching, remove any local tags that no longer exist on
+   the remote if `--prune` is enabled. This option should be used
+   more carefully, unlike `--prune` it will remove any local
+   references 

[PATCH v3 10/11] fetch tests: add scaffolding for the new fetch.pruneTags

[Ævar Arnfjörð Bjarmason]

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

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

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index e5e88ee474..840fd5ef02 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -551,18 +551,22 @@ set_config_tristate () {
 test_configured_prune () {
fetch_prune=$1
remote_origin_prune=$2
-   expected_branch=$3
-   expected_tag=$4
-   cmdline=$5
+   fetch_prune_tags=$3
+   remote_origin_prune_tags=$4
+   expected_branch=$5
+   expected_tag=$6
+   cmdline=$7
 
-   test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${5:+ 
$5}; branch:$3 tag:$4" '
+   test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2 
fetch.pruneTags=$3 remote.origin.pruneTags=$4${7:+ $7}; branch:$5 tag:$6" '
# make sure a newbranch is there in . and also in one
git branch -f newbranch &&
git tag -f newtag &&
(
cd one &&
test_unconfig fetch.prune &&
+   test_unconfig fetch.pruneTags &&
test_unconfig remote.origin.prune &&
+   test_unconfig remote.origin.pruneTags &&
git fetch &&
git rev-parse --verify refs/remotes/origin/newbranch &&
git rev-parse --verify refs/tags/newtag
@@ -576,7 +580,9 @@ test_configured_prune () {
(
cd one &&
set_config_tristate fetch.prune $fetch_prune &&
+   set_config_tristate fetch.pruneTags $fetch_prune_tags &&
set_config_tristate remote.origin.prune 
$remote_origin_prune &&
+   set_config_tristate remote.origin.pruneTags 
$remote_origin_prune_tags &&
 
git fetch $cmdline &&
case "$expected_branch" in
@@ -601,57 +607,59 @@ test_configured_prune () {
 
 # $1 config: fetch.prune
 # $2 config: remote..prune
-# $3 expect: branch to be pruned?
-# $4 expect: tag to be pruned?
-# $5 git-fetch $cmdline:
+# $3 config: fetch.pruneTags
+# $4 config: remote..pruneTags
+# $5 expect: branch to be pruned?
+# $6 expect: tag to be pruned?
+# $7 git-fetch $cmdline:
 #
-# $1$2$3 $4 $5
-test_configured_prune unset unset kept   kept   ""
-test_configured_prune unset unset kept   kept   "--no-prune"
-test_configured_prune unset unset pruned kept   "--prune"
-test_configured_prune unset unset kept   pruned \
+# $1$2$3$4$5 $6 $7
+test_configured_prune unset unset unset unset kept   kept   ""
+test_configured_prune unset unset unset unset kept   kept   "--no-prune"
+test_configured_prune unset unset unset unset pruned kept   "--prune"
+test_configured_prune unset unset unset unset kept   pruned \
"--prune origin +refs/tags/*:refs/tags/*"
-test_configured_prune unset unset pruned pruned \
+test_configured_prune unset unset unset unset pruned pruned \
"--prune origin +refs/tags/*:refs/tags/* 
+refs/heads/*:refs/remotes/origin/*"
 
-test_configured_prune false unset kept   kept   ""
-test_configured_prune false unset kept   kept   "--no-prune"
-test_configured_prune false unset pruned kept   "--prune"
+test_configured_prune false unset unset unset kept   kept   ""
+test_configured_prune false unset unset unset kept   kept   "--no-prune"
+test_configured_prune false unset unset unset pruned kept   "--prune"
 
-test_configured_prune true  unset pruned kept   ""
-test_configured_prune true  unset pruned kept   "--prune"
-test_configured_prune true  unset kept   kept   "--no-prune"
+test_configured_prune true  unset unset unset pruned kept   ""
+test_configured_prune true  unset unset unset pruned kept   "--prune"
+test_configured_prune true  unset unset unset kept   kept   "--no-prune"
 
-test_configured_prune unset false kept   kept   ""
-test_configured_prune unset false kept   kept   "--no-prune"
-test_configured_prune unset false pruned kept   "--prune"
+test_configured_prune unset false unset unset kept   kept   ""
+test_configured_prune unset false unset unset kept   kept   "--no-prune"
+test_configured_prune unset false unset unset pruned kept   "--prune"
 
-test_configured_prune false false kept   kept   ""
-test_configured_prune false false kept   kept   "--no-prune"
-test_configured_prune false false pruned kept   "--prune"
-test_configured_prune false false kept   pruned \
+test_configured_prune false false unset unset 

[PATCH v3 09/11] git-fetch & config doc: link to the new PRUNING section

[Ævar Arnfjörð Bjarmason]

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

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

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

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

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

[PATCH v3 05/11] fetch tests: add a tag to be deleted to the pruning tests

[Ævar Arnfjörð Bjarmason]

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

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

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

[PATCH v3 07/11] git fetch doc: add a new section to explain the ins & outs of pruning

[Ævar Arnfjörð Bjarmason]

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

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

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

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

[PATCH v3 02/11] fetch: stop accessing "remote" variable indirectly

[Ævar Arnfjörð Bjarmason]

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

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

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

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

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

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

[PATCH v3 03/11] fetch tests: refactor in preparation for testing tag pruning

[Ævar Arnfjörð Bjarmason]

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

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

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

Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog

[Lucas Werkmeister]

Thanks for your responses!

On 23.01.2018 01:00, Ævar Arnfjörð Bjarmason wrote:
> This patch looks good, but I wonder if with the rise of systemd
> there's a good reason to flip the default around to not having other
> stuff imply --syslog, and have users specify this implictly, then we
> won't need a --no-syslog option.
> 
> But maybe that'll break too much stuff.
> 

That seems risky to me – even with systemd, the StandardError directive
by default inherits StandardOutput, so if you set StandardOutput=socket
without StandardError=journal, log output in stderr clobbers regular
output. (Also, stderr is apparently closed with --detach, see below.)

On 23.01.2018 19:30, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  writes:
> 
>> On Mon, Jan 22 2018, Lucas Werkmeister jotted:
>> 
>>> Several options imply --syslog, without there being a way to
>>> disable it again. This commit adds that option.
>> 
>> Just two options imply --syslog, --detach & --inetd, unless I've
>> missed something, anyway 2 != several, so maybe just say "The
>> --detach and --inetd options imply --syslog ...".
> 
> Correct.

I respectfully disagree on “2 != several”, but sure, I can repeat the
two options in the message instead :)

> Moreover, --detach completely dissociates the process from the
> original set of standard file descriptors by first closing them and
> then connecting it to "/dev/null", so it will be nonsense to use this
> new option with it.
> 

Ah, I wasn’t aware of that – so with --detach, --no-syslog would be
better described as “disables all logging” rather than “log to stderr
instead”. IMHO it would still make sense to have the --no-syslog option
(then mainly for use with --inetd) as long as its interaction with
--inetd is properly documented… do you agree? If yes, I’ll be glad to
submit another version of the patch.

Best regards,
Lucas



smime.p7s
Description: S/MIME Cryptographic Signature

HELLO

[Mr. Hamza Kabore]

-- 
Hello Dear Friend,

Greetings and how are you doing?
I want to know if you are keen to be my partner in claiming the
fortune $12.8 Million USD left by a late client. If you're interested
revert for more details.

Awaiting your reply

Hamza Kabore

Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute

[Junio C Hamano]

Jeff King  writes:

> But with Coccinelle, it's a lot easier to apply the change tree-wide, and
> to convert topics in flight as they get merged. The maintainer still
> gets conflicts with topics-in-flight that touch converted areas, though.
> So I'd be curious to hear if Junio's opinion has changed at all.

There are two distinct kinds of cost on such a tree-wide change.
Conflicts with in-flight topic cannot be avoided other than truly
avoiding, i.e. refraining from touching the areas in flux, but it is
primarily what the maintainer does, and with help with rerere it can
be reasonably well automated ;-)

But the cost of reviewing could become a lot smaller when our tools
are trustworthy.  As long as we can be reasonably certain that the
tree-wide change patch does one thing it is intended to do and
nothing else (e.g. comes with mechanical reproduction recipe that
allows the patch to be independently audited), I do not have much
problem with such a clean-up.

The "avoid tree-wide change" rule still applies for things that
allows a lot of subjective judgment and discretion.  I do not know
of a good way to reduce reviewer costs on those kind of changes.

Thanks.

Re: [PATCH v4 0/6] convert: add support for different encodings

[Junio C Hamano]

Torsten Bögershausen  writes:

> On Sat, Jan 20, 2018 at 04:24:12PM +0100, lars.schnei...@autodesk.com wrote:
>> From: Lars Schneider 
>> 
>> Hi,
>> 
>> Patches 1-4 and 6 are preparation and helper functions.
>> Patch 5 is the actual change.
>
> I (still) have 2 remarks on convert.c - to make live easier,
> I will send a small "on top" patch the next days.

Thanks, both.  I'll stay on the sideline ;-) and deal with other
topics first.

RE: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[Randall S. Becker]

On January 23, 2018 1:13 PM, Junio C Hamano wrote:
> "Randall S. Becker"  writes:
> 
> >> IOW, I do not see it explained clearly why this change is needed on
> >> any single platform---so "that issue may be shared by others, too"
> >> is a bit premature thing for me to listen to and understand, as "that
> >> issue" is quite unclear to me.
> >
> > v4 might be a little better. The issue seems to be specific to NonStop
> > that it's PIPE mechanism needs to have setbuf(pipe,NULL) called for
> > git to be happy.  The default behaviour appears to be different on
> > NonStop from other platforms from our testing. We get hung up waiting
> > on pipes unless this is done.
> 
> I am afraid that that is not a "diagnosis" enough to allow us moving forward.
> We get hung up because...?  When the process that has the other end of
> pipe open exits, NonStop does not close the pipe properly?  Or NonStop
> does not flush the data buffered in the pipe?
> Would it help if a compat wrapper that does setbuf(fd, NULL) immediately
> before closing the fd, or some other more targetted mechanism, is used only
> on NonStop, for example?  Potentially megabytes of data can pass thru a
> pipe, and if the platform bug affects only at the tail end of the transfer,
> marking the pipe not to buffer at all at the beginning is too big a hammer to
> work it around.  With the explanation given so far, this still smells more 
> like
> "we have futzed around without understanding why, and this happens to
> work."  It may be good enough for your purpose of making progress (after
> all, I'd imagine that you'd need to work this around one way or another to
> hunt for and fix more issues on the platform), but it does not sound like "we
> know what the problem is, and this is the best workaround for that", which is
> what we want if it wants to become part of the official codebase.

I am retesting without setbuf(NULL) but this is unlikely to be enlightening. 
The test cases do not adequately simulate the configuration in which my team 
originally encountered the problem. This requires a guarantee of the source and 
destination coming through different logical CPUs. We never encountered the 
issue in the test suite, only when end users got hold of it. We had two 
distinct problems, one which was the revent=0 related hang (already solved) and 
other was a stream flush problem. The two are related but distinct. The 
platform support group insisted that we should have the setbuf(NULL) in place 
for interprocess communications in the form used here - I'm worried about 
losing this, but completely aware that this is far too heavy for other 
platforms (hence the __TANDEM guard in wrapper.c). If the form of the wrapper 
should be different, I would be happy to comply.

I have a much longer explanation about the platform message stack structure, 
but that doesn't belong here. Happy to respond privately if requested.

Cheers,
Randall

-- Brief whoami:
 NonStop developer since approximately 2112884442
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [PATCH 0/8] rebase -i: offer to recreate merge commits

[Junio C Hamano]

Johannes Schindelin  writes:

> My original attempt was --preserve-merges, but that design was so
> limited that I did not even enable it in interactive mode.
> ...
> There are more patches in the pipeline, based on this patch series, but
> left for later in the interest of reviewable patch series: one mini
> series to use the sequencer even for `git rebase -i --root`, and another
> one to add support for octopus merges to --recreate-merges.

I left comments on a handful of them, but I do not think any of them
spotted a grave design issue to be a show stopper.  Overall, the
series was quite a pleasant read, even with those minor nits and
rooms for improvements.

Thanks.

Re: [PATCH 6/8] sequencer: handle autosquash and post-rewrite for merge commands

[Junio C Hamano]

Jacob Keller  writes:

>>  static int is_per_worktree_ref(const char *refname)
>>  {
>> return !strcmp(refname, "HEAD") ||
>> -   starts_with(refname, "refs/bisect/");
>> +   starts_with(refname, "refs/bisect/") ||
>> +   starts_with(refname, "refs/rewritten/");
>>  }
>
> Would this part make more sense to move into the commit that
> introduces writing these refs, or does it only matter once you start
> this step here?

Good spotting.  I too was wondering about multiple worktrees when I
saw the "label" thing introduced.  It probably makes sense to move
this to that step.

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

[Junio C Hamano]

Johannes Schindelin  writes:

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 8a861c1e0d6..1d061373288 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -368,6 +368,11 @@ The commit list format can be changed by setting the 
> configuration option
>  rebase.instructionFormat.  A customized instruction format will automatically
>  have the long commit hash prepended to the format.
>  
> +--recreate-merges::
> + Recreate merge commits instead of flattening the history by replaying
> + merges. Merge conflict resolutions or manual amendments to merge
> + commits are not preserved.
> +

It is sensible to postpone tackling "evil merges" in this initial
iteration of the series, and "manual amendments ... not preserved"
is a reasonable thing to document.  But do we want to say a bit more
about conflicting merges?  "conflict resolutions ... not preserved"
sounds as if it does not stop and instead record the result with
conflict markers without even letting rerere to kick in, which
certainly is not the impression you wanted to give to the readers.

I am imagining that it will stop and give control back to the end
user just like a conflicted "pick" would, and allow "rebase
--continue" to record resolution from the working tree, and just
like conflicted "pick", it would allow rerere() to help end users
recall previous resolution.

Re: [PATCH v4 0/6] convert: add support for different encodings

[Torsten Bögershausen]

On Sat, Jan 20, 2018 at 04:24:12PM +0100, lars.schnei...@autodesk.com wrote:
> From: Lars Schneider 
> 
> Hi,
> 
> Patches 1-4 and 6 are preparation and helper functions.
> Patch 5 is the actual change.

I (still) have 2 remarks on convert.c - to make live easier,
I will send a small "on top" patch the next days.

Re: [PATCH 4/8] rebase-helper --make-script: introduce a flag to recreate merges

[Junio C Hamano]

Eric Sunshine  writes:

>> +   is_octopus = to_merge && to_merge->next;
>> +
>> +   if (is_octopus)
>> +   BUG("Octopus merges not yet supported");
>
> Is this a situation which the end-user can trigger by specifying a
> merge with more than two parents? If so, shouldn't this be just a
> normal error message rather than a (developer) bug message? Or, am I
> misunderstanding?

BUG() is "we wrote code carefully so that this should not trigger;
we do not _expect_ the code to reach here".  This one is expected to
trigger, and I agree with you that it should be die(), if the series
is meant to be released to the general public in the current form
(i.e. until the limitation is lifted so that it can handle an
octopus).

If the callers are made more careful to check if there is an octopus
involved and reject the request early, then seeing an octopus in
this location in a loop will become a BUG().

Re: [PATCH 4/8] rebase-helper --make-script: introduce a flag to recreate merges

[Junio C Hamano]

Johannes Schindelin  writes:

> The sequencer just learned a new commands intended to recreate branch

s/a //;

> structure (similar in spirit to --preserve-merges, but with a
> substantially less-broken design).
> ...
> @@ -2785,6 +2787,335 @@ void append_signoff(struct strbuf *msgbuf, int 
> ignore_footer, unsigned flag)
>   strbuf_release();
>  }
>  
> +struct labels_entry {
> + struct hashmap_entry entry;
> + char label[FLEX_ARRAY];
> +};
> +
> +static int labels_cmp(const void *fndata, const struct labels_entry *a,
> +   const struct labels_entry *b, const void *key)
> +{
> + return key ? strcmp(a->label, key) : strcmp(a->label, b->label);
> +}

label_oid() accesses state->labels hash using strihash() as the hash
function, but the final comparison between the entries in the same
hash buckets are done with case sensitivity.  It is unclear to me if
that is what was intended, and why.

> +struct string_entry {
> + struct oidmap_entry entry;
> + char string[FLEX_ARRAY];
> +};
> +
> +struct label_state {
> + struct oidmap commit2label;
> + struct hashmap labels;
> + struct strbuf buf;
> +};
> +
> +static const char *label_oid(struct object_id *oid, const char *label,
> +  struct label_state *state)
> +{
> + struct labels_entry *labels_entry;
> + struct string_entry *string_entry;
> + struct object_id dummy;
> + size_t len;
> + int i;
> +
> + string_entry = oidmap_get(>commit2label, oid);
> + if (string_entry)
> + return string_entry->string;
> +
> + /*
> +  * For "uninteresting" commits, i.e. commits that are not to be
> +  * rebased, and which can therefore not be labeled, we use a unique
> +  * abbreviation of the commit name. This is slightly more complicated
> +  * than calling find_unique_abbrev() because we also need to make
> +  * sure that the abbreviation does not conflict with any other
> +  * label.
> +  *
> +  * We disallow "interesting" commits to be labeled by a string that
> +  * is a valid full-length hash, to ensure that we always can find an
> +  * abbreviation for any uninteresting commit's names that does not
> +  * clash with any other label.
> +  */
> + if (!label) {
> + char *p;
> +
> + strbuf_reset(>buf);
> + strbuf_grow(>buf, GIT_SHA1_HEXSZ);
> + label = p = state->buf.buf;
> +
> + find_unique_abbrev_r(p, oid->hash, default_abbrev);
> +
> + /*
> +  * We may need to extend the abbreviated hash so that there is
> +  * no conflicting label.
> +  */
> + if (hashmap_get_from_hash(>labels, strihash(p), p)) {
> + size_t i = strlen(p) + 1;
> +
> + oid_to_hex_r(p, oid);
> + for (; i < GIT_SHA1_HEXSZ; i++) {
> + char save = p[i];
> + p[i] = '\0';
> + if (!hashmap_get_from_hash(>labels,
> +strihash(p), p))
> + break;
> + p[i] = save;
> + }
> + }

If oid->hash required full 40-hex to disambiguate, then
find-unique-abbrev would give 40-hex and we'd want the same "-"
suffix technique employed below to make it consistently unique.  I
wonder if organizing the function this way ...

if (!label)
label = oid-to-hex(oid);

if (label already exists or full oid) {
make it unambiguous;
}

... allows the resulting code easier to understand and manage.

A related tangent.  Does an auto-label given to "uninteresting"
commit need to be visible to end users?  I doubted it and that is
why I said oid-to-hex in the above, but if it is given to end users,
use of find-unique-abbrev-r is perfectly fine.

> +static int make_script_with_merges(struct pretty_print_context *pp,
> +struct rev_info *revs, FILE *out,
> +unsigned flags)
> +{
> + ...
> + int abbr = flags & TODO_LIST_ABBREVIATE_CMDS;
> + const char *p = abbr ? "p" : "pick", *l = abbr ? "l" : "label",
> +  *t = abbr ? "t" : "reset", *b = abbr ? "b" : "bud",
> +  *m = abbr ? "m" : "merge";

It would be easier to understand if these short named variables are
reserved only for temporary use, not as constants.  It is not too
much to spell 

fprintf(out, "%s onto\n", cmd_label);

than

fprintf(out, "%s onto\n", l);

and would save readers from head-scratching, wondering where the
last assignment to variable "l" is.

> +
> + oidmap_init(, 0);
> + oidmap_init(, 0);
> + hashmap_init(, (hashmap_cmp_fn) labels_cmp, NULL, 0);
> + strbuf_init(, 32);
> +
> + if (revs->cmdline.nr && 

Re: [PATCH 3/8] sequencer: fast-forward merge commits, if possible

[Junio C Hamano]

Phillip Wood  writes:

> On 18/01/18 15:35, Johannes Schindelin wrote:
>> 
>> Just like with regular `pick` commands, if we are trying to recreate a
>> merge commit, we now test whether the parents of said commit match HEAD
>> and the commits to be merged, and fast-forward if possible.
>> 
>> This is not only faster, but also avoids unnecessary proliferation of
>> new objects.
>
> I might have missed something but shouldn't this be checking opts->allow_ff?

Because the whole point of this mechanism is to recreate the
topology faithfully to the original, even if the original was a
redundant merge (which has a side parent that is an ancestor or a
descendant of the first parent), we should just point at the
original merge when the condition allows it, regardless of
opts->allow_ff.

I think it is a different matter if an insn to create a new merge
(i.e. "merge -  ", not "merge  ")
should honor opts->allow_ff; because it is not about recreating an
existing history but is a way to create what did not exist before,
I think it is sensible if allow_ff option is honored.

Re: [PATCH 3/8] sequencer: fast-forward merge commits, if possible

[Junio C Hamano]

Johannes Schindelin  writes:

> + /*
> +  * If HEAD is not identical to the parent of the original merge commit,
> +  * we cannot fast-forward.
> +  */
> + can_fast_forward = commit && commit->parents &&
> + !oidcmp(>parents->item->object.oid,
> + _commit->object.oid);
> +

I think this expression and assignment should better be done much
later.  Are you going to update commit, commit->parents, etc. that
are involved in the computation in the meantime???

>   strbuf_addf(_name, "refs/rewritten/%.*s", merge_arg_len, arg);
>   merge_commit = lookup_commit_reference_by_name(ref_name.buf);
>   if (!merge_commit) {
> @@ -2164,6 +2172,17 @@ static int do_merge(struct commit *commit, const char 
> *arg, int arg_len,
>   rollback_lock_file();
>   return -1;
>   }
> +
> + if (can_fast_forward && commit->parents->next &&
> + !commit->parents->next->next &&
> + !oidcmp(>parents->next->item->object.oid,
> + _commit->object.oid)) {

... Namely, here.  Because the earlier one is computing "are we
replaying exactly the same commit on top of exactly the same
state?", which is merely one half of "can we fast-forward", and
storing it in a variable whose name is over-promising way before it
becomes necessary.  The other half of "can we fast-forward?" logic
is the remainder of the if() condition we see above.  IOW, when
fully spelled, this code can fast-forward when we are replaying a
commit on top of exactly the same first-parent and the commit being
replayed is a single parent merge.

We may even want to get rid of can_fast_forward variable.

> + strbuf_release(_name);
> + rollback_lock_file();
> + return fast_forward_to(>object.oid,
> +_commit->object.oid, 0, opts);
> + }
> +
>   write_message(oid_to_hex(_commit->object.oid), GIT_SHA1_HEXSZ,
> git_path_merge_head(), 0);
>   write_message("no-ff", 5, git_path_merge_mode(), 0);

Re: [PATCH v2 1/2] t7505: Add tests for cherry-pick and rebase -i/-p

[Junio C Hamano]

Phillip Wood  writes:

> + export GIT_SEQUENCE_EDITOR GIT_EDITOR &&
> + test_must_fail git rebase $mode b &&
> + echo x>a &&

"echo x >a"

Re: [PATCH v2 1/2] t7505: Add tests for cherry-pick and rebase -i/-p

[Junio C Hamano]

Phillip Wood  writes:

> @@ -31,17 +63,40 @@ mkdir -p "$HOOKDIR"
>  echo "#!$SHELL_PATH" > "$HOOK"
>  cat >> "$HOOK" <<'EOF'
>  
> +GIT_DIR=$(git rev-parse --git-dir)
> +if test -d "$GIT_DIR/rebase-merge"
> +then
> +  rebasing=1
> +else
> +  rebasing=0
> +fi
> +
> +get_last_cmd () {
> +  tail -n1 "$GIT_DIR/rebase-merge/done" | {
> +read cmd id _
> +git log --pretty="[$cmd %s]" -n1 $id
> +  }
> +}
> +
>  if test "$2" = commit; then
> -  source=$(git rev-parse "$3")
> +  if test $rebasing = 1
> +  then
> +source="$3"
> +  else
> +source=$(git rev-parse "$3")
> +  fi
>  else
>source=${2-default}
>  fi
> -if test "$GIT_EDITOR" = :; then
> -  sed -e "1s/.*/$source (no editor)/" "$1" > msg.tmp
> +test "$GIT_EDITOR" = : && source="$source (no editor)"
> +
> +if test $rebasing = 1
> +then
> +  echo "$source $(get_last_cmd)" >"$1"
>  else
>sed -e "1s/.*/$source/" "$1" > msg.tmp
> +  mv msg.tmp "$1"
>  fi

It is somewhat irritating that indentation is screwed up in this
part of the file.  Can we not make it even worse?

Re: The original file that was split in 2 other files, is there a way in git to see what went where?

[Junio C Hamano]

Jeff King  writes:

> On Mon, Jan 22, 2018 at 10:22:21PM -0500, Aleksey Bykov wrote:
>
>> I am a code reviewer, I have a situation in GIT:
>> 
>> - before: a.txt
>> 
>> Then a developer decided to split the content of a.txt into 2 files
>> and add a few changes all in one commit:
>> 
>> - after: b.txt + few changes and c.txt + few changes
>> ...
> For seeing which line came from where, you might try "git blame -C",
> which will cross file boundaries looking for the source of lines.
> ...
> And finally, if you're going to do a lot with "git blame", I'd look into
> the "tig" tool as a prettier interface. You should be able to do "tig
> blame -C ..." in the same way.

All excellent guides.  "blame" is good at explaining where things
came from, but not as good at explaining, starting from an old
state, where things went.  "blame --reverse" does a decent job
within the constraints its output format has, but not quite ideal.

Re: [PATCH] Fixes compile warning with -Wimplicit-fallthrough CFLAGS

[Jeff King]

On Tue, Jan 23, 2018 at 10:33:57AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >> diff --git a/apply.c b/apply.c
> >> index 321a9fa68..a22fb2881 100644
> >> --- a/apply.c
> >> +++ b/apply.c
> >> @@ -1450,7 +1450,7 @@ static void recount_diff(const char *line, int size, 
> >> struct fragment *fragment)
> >>switch (*line) {
> >>case ' ': case '\n':
> >>newlines++;
> >> -  /* fall through */
> >> +  GIT_FALLTHROUGH;
> >
> > Ugh, the semi-colon there makes it look like it's actual code. If we go
> > this route, I wonder if it's worth hiding it inside the macro.
> 
> What?  You mean to shout in all caps without even terminating the
> line with any punctuation?  Please don't---I am sure it will break
> auto indentation people rely on from their editors.

True, that may be even worse. I just wonder if we can do something to
make it look more obviously like a non-code attribute. The actual syntax
is something like:

  [[fallthrough]];

which is pretty horrid, but at least a bit easier to see. gcc also
provides "__attribute__((fallthrough))", but I don't think it works with
clang.

I vastly prefer the comment approach if we can use it. Apparently clang
doesn't support it, but I have also not managed to get clang (either
version 4, 6, or the upcoming 7) to actually report anything via
-Wimplicit-fallthrough, either. Maybe I'm holding it wrong.

-Peff

Re: [PATCH] Fixes compile warning with -Wimplicit-fallthrough CFLAGS

[Junio C Hamano]

Jeff King  writes:

>> diff --git a/apply.c b/apply.c
>> index 321a9fa68..a22fb2881 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -1450,7 +1450,7 @@ static void recount_diff(const char *line, int size, 
>> struct fragment *fragment)
>>  switch (*line) {
>>  case ' ': case '\n':
>>  newlines++;
>> -/* fall through */
>> +GIT_FALLTHROUGH;
>
> Ugh, the semi-colon there makes it look like it's actual code. If we go
> this route, I wonder if it's worth hiding it inside the macro.

What?  You mean to shout in all caps without even terminating the
line with any punctuation?  Please don't---I am sure it will break
auto indentation people rely on from their editors.

Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Mon, Jan 22 2018, Lucas Werkmeister jotted:
>
>> Several options imply --syslog, without there being a way to disable it
>> again. This commit adds that option.
>
> Just two options imply --syslog, --detach & --inetd, unless I've missed
> something, anyway 2 != several, so maybe just say "The --detach and
> --inetd options imply --syslog ...".

Correct. Moreover, --detach completely dissociates the process from
the original set of standard file descriptors by first closing them
and then connecting it to "/dev/null", so it will be nonsense to use
this new option with it.

Re: [PATCH] sequencer: mark a file local symbol as static

[Junio C Hamano]

Ramsay Jones  writes:

> Signed-off-by: Ramsay Jones 
> ---
>
> Hi Phillip,
>
> If you need to re-roll your 'pw/sequencer-in-process-commit' branch, could
> you please squash this into the relevant patch (commit da96adcf5a,
> "sequencer: run 'prepare-commit-msg' hook", 2018-01-19).
>
> Thanks.

Thanks; as the commit is sitting at the tip of the topic not yet in 'next',
let me just squash it in.

>
> ATB,
> Ramsay Jones
>
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 79579ba11..5bfdc4044 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -893,7 +893,7 @@ void commit_post_rewrite(const struct commit *old_head,
>   run_rewrite_hook(_head->object.oid, new_head);
>  }
>  
> -int run_prepare_commit_msg_hook(struct strbuf *msg, const char *commit)
> +static int run_prepare_commit_msg_hook(struct strbuf *msg, const char 
> *commit)
>  {
>   struct argv_array hook_env = ARGV_ARRAY_INIT;
>   int ret;

Re: [PATCH] fsck: fix leak when traversing trees

[Junio C Hamano]

Eric Wong  writes:

> While fsck_walk/fsck_walk_tree/parse_tree populates "struct tree"
> idempotently, it is still up to the fsck_walk caller to call
> free_tree_buffer.
>
> Fixes: ad2db4030e42890e ("fsck: remove redundant parse_tree() invocation")

Yup, we can see that that commit did stop freeing the tree buffer.
Thanks.

>
> Signed-off-by: Eric Wong 
> ---
>  These APIs could probably be made to be less error-prone,
>  but at least this stops my little machine from OOM-ing.
>
>  builtin/fsck.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 04846d46f9..92ce775a74 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -171,7 +171,13 @@ static void mark_object_reachable(struct object *obj)
>  
>  static int traverse_one_object(struct object *obj)
>  {
> - return fsck_walk(obj, obj, _walk_options);
> + int result = fsck_walk(obj, obj, _walk_options);
> +
> + if (obj->type == OBJ_TREE) {
> + struct tree *tree = (struct tree *)obj;
> + free_tree_buffer(tree);
> + }
> + return result;
>  }
>  
>  static int traverse_reachable(void)

Re: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[Junio C Hamano]

"Randall S. Becker"  writes:

>> IOW, I do not see it explained clearly why this change is needed on any 
>> single
>> platform---so "that issue may be shared by others, too"
>> is a bit premature thing for me to listen to and understand, as "that issue" 
>> is
>> quite unclear to me.
>
> v4 might be a little better. The issue seems to be specific to
> NonStop that it's PIPE mechanism needs to have setbuf(pipe,NULL)
> called for git to be happy.  The default behaviour appears to be
> different on NonStop from other platforms from our testing. We get
> hung up waiting on pipes unless this is done.

I am afraid that that is not a "diagnosis" enough to allow us moving
forward.  We get hung up because...?  When the process that has the
other end of pipe open exits, NonStop does not close the pipe
properly?  Or NonStop does not flush the data buffered in the pipe?
Would it help if a compat wrapper that does setbuf(fd, NULL)
immediately before closing the fd, or some other more targetted
mechanism, is used only on NonStop, for example?  Potentially
megabytes of data can pass thru a pipe, and if the platform bug
affects only at the tail end of the transfer, marking the pipe not
to buffer at all at the beginning is too big a hammer to work it
around.  With the explanation given so far, this still smells more
like "we have futzed around without understanding why, and this
happens to work."  It may be good enough for your purpose of making
progress (after all, I'd imagine that you'd need to work this around
one way or another to hunt for and fix more issues on the platform),
but it does not sound like "we know what the problem is, and this is
the best workaround for that", which is what we want if it wants to
become part of the official codebase.

Re: The original file that was split in 2 other files, is there a way in git to see what went where?

[Jeff King]

On Mon, Jan 22, 2018 at 10:22:21PM -0500, Aleksey Bykov wrote:

> I am a code reviewer, I have a situation in GIT:
> 
> - before: a.txt
> 
> Then a developer decided to split the content of a.txt into 2 files
> and add a few changes all in one commit:
> 
> - after: b.txt + few changes and c.txt + few changes
> 
> Is there an easy way to see:
> 
> 1. what came to b from a?
> 2 .what came to c from a?
> 3. all extra changes apart from just moving stuff?

Jonathan suggested the new "--color-moved", which I second as a good way
of seeing what was moved, and which lines were changed.

For seeing which line came from where, you might try "git blame -C",
which will cross file boundaries looking for the source of lines.

For instance, here's a case in git where some code was moved:

  git blame -C ae563542bf10fa8c33abd2a354e4b28aca4264d7 revision.c

You can see which lines are new to the file, and which ones were moved
from elsewhere.

If you want to simplify the "noise" of seeing the actual origin of each
line, you can ask blame not to go further back. Like:

  commit=ae563542bf10fa8c33abd2a354e4b28aca4264d7
  git blame -b -C $commit^..$commit revision.c

That will leave the commit id blank for every line that wasn't touched
as part of the commit (or if you had a whole series of commits, replace
"$commit^" with the parent of the series).

And finally, if you're going to do a lot with "git blame", I'd look into
the "tig" tool as a prettier interface. You should be able to do "tig
blame -C ..." in the same way.

-Peff

Re: [PATCH 5/5] travis-ci: don't fail if user already exists on 32 bit Linux build job

[Jeff King]

On Mon, Jan 22, 2018 at 02:32:20PM +0100, SZEDER Gábor wrote:

> The 32 bit Linux build job runs in a Docker container, which lends
> itself to running and debugging locally, too.  Especially during
> debugging one usually doesn't want to start with a fresh container
> every time, to save time spent on installing a bunch of dependencies.
> However, that doesn't work quite smootly, because the script running
> in the container always creates a new user, which then must be removed
> every time before subsequent executions, or the build script fails.
> 
> Make this process more convenient and don't try to create that user if
> it already exists and has the right user ID in the container, so
> developers don't have to bother with running a 'userdel' each time
> before they run the build script.

Makes sense.

> The build job on Travis CI always starts with a fresh Docker
> container, so this change doesn't make a difference there.

I wonder if that is fixable. Installing dependencies into the container
takes quite a lot of time, and is just wasted effort.

> diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
> index e37e1d2d5f..13047adde3 100755
> --- a/ci/run-linux32-build.sh
> +++ b/ci/run-linux32-build.sh
> @@ -33,7 +33,13 @@ then
>   CI_USER=root
>  else
>   CI_USER=ci
> - useradd -u $HOST_UID $CI_USER
> + if test "$(id -u $CI_USER)" = $HOST_UID
> + then
> + : # user already exists with the right ID
> + else
> + useradd -u $HOST_UID $CI_USER
> + fi

Is it worth redirecting the stderr of "id" to avoid noise when $CI_USER
does not yet exist at all? Or is that a useful log message? :)

-Peff

Re: [PATCH 4/5] travis-ci: don't run the test suite as root in the 32 bit Linux build

[Jeff King]

On Mon, Jan 22, 2018 at 02:32:19PM +0100, SZEDER Gábor wrote:

> Travis CI runs the 32 bit Linux build job in a Docker container, where
> all commands are executed as root by default.  Therefore, ever since
> we added this build job in 88dedd5e7 (Travis: also test on 32-bit
> Linux, 2017-03-05), we have a bit of code to create a user in the
> container matching the ID of the host user and then to run the test
> suite as this user.  Matching the host user ID is important, because
> otherwise the host user would have no access to any files written by
> processes running in the container, notably the logs of failed tests
> couldn't be included in the build job's trace log.
> 
> Alas, this piece of code never worked, because it sets the variable
> holding the user name ($CI_USER) in a subshell, meaning it doesn't
> have any effect by the time we get to the point to actually use the
> variable to switch users with 'su'.  So all this time we were running
> the test suite as root.

This all makes sense to me, and the patch looks sane.

> Reorganize that piece of code in 'ci/run-linux32-build.sh' a bit to
> avoid that problematic subshell and to ensure that we switch to the
> right user.  Furthermore, make the script's optional host user ID
> option mandatory, so running the build accidentally as root will
> become harder when debugging locally.  If someone really wants to run
> the test suite as root, whatever the reasons might be, it'll still be
> possible to do so by explicitly passing '0' as host user ID.

Coincidentally, we recently set up a docker test build for our local fork
of Git. We found the whole "mount the existing git source tree" strategy
slightly annoying, exactly because of these mismatches between the host
and docker uids.

Instead, we ended up with something more like:

  git archive HEAD | docker run ...

and the in-container script starts with:

  tar -x

to extract the to-be-tested source, and ends with a dump of the failures
to stdout.

I don't know if it's worth switching strategies now, but just some food
for thought. It may be less interesting with Travis, too, because you're
also trying to carry the prove cache across runs, which does require
some filesystem interaction.

(As an aside, I'm not sure the prove cache is doing much. Running in
slow-to-fast order helps if you are trying to run massively in parallel,
but we only use -j3 for our Travis builds).

-Peff

Re: The original file that was split in 2 other files, is there a way in git to see what went where?

[Jonathan Tan]

On Mon, Jan 22, 2018 at 7:22 PM, Aleksey Bykov  wrote:
> Is there an easy way to see:
>
> 1. what came to b from a?
> 2 .what came to c from a?
> 3. all extra changes apart from just moving stuff?

One way to do this is to use "--color-moved" - it will tell you what
in b.txt and c.txt was moved and what wasn't (although it won't tell
you from where). This was introduced recently, I think in 2.15.

Re: [PATCH 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job

[Jeff King]

On Tue, Jan 23, 2018 at 11:26:33AM -0500, Jeff King wrote:

> > +HOST_UID=$1
> > +CI_USER=$USER
> > +test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER)
> 
> If this "useradd" step fails, we wouldn't abort the script, because it's
> part of a conditional. You'd need a manual "|| exit 1" at the end of
> this line. Or to use a real "if" block.
> 
> Reading this line, I'm also slightly confused by setting CI_USER in the
> subshell, and then only using it once. Should it be respecting the outer
> CI_USER setting? If not, should it just hard-code "ci" on the useradd
> command-line?  But that has nothing to do with your patch here.

OK, nevermind on all this. I just read patch 4. :)

-Peff

Re: [PATCH 3/5] travis-ci: don't repeat the path of the cache directory

[Jeff King]

On Mon, Jan 22, 2018 at 02:32:18PM +0100, SZEDER Gábor wrote:

> Some of our 'ci/*' scripts repeat the name or full path of the Travis
> CI cache directory, and the following patches will add new places
> using that path.
> 
> Use a variable to refer to the path of the cache directory instead, so
> it's hard-coded only in a single place.
> 
> Pay extra attention to the 32 bit Linux build: it runs in a Docker
> container, so pass the path of the cache directory from the host to
> the container in an environment variable.  Furthermore, use the
> variable in the container only if it's set, to prevent errors when
> someone is running or debugging the Docker build locally, because in
> that case the variable won't be set as there won't be any Travis CI
> cache.

Makes sense that we need to pass it down, but...

> diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
> index 248183982b..c9476d6598 100755
> --- a/ci/run-linux32-build.sh
> +++ b/ci/run-linux32-build.sh
> @@ -25,10 +25,10 @@ CI_USER=$USER
>  test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER)
>  
>  # Build and test
> -linux32 --32bit i386 su -m -l $CI_USER -c '
> +linux32 --32bit i386 su -m -l $CI_USER -c "
>   set -ex
>   cd /usr/src/git
> - ln -s /tmp/travis-cache/.prove t/.prove
> + test -n '$cache_dir' && ln -s '$cache_dir/.prove' t/.prove
>   make --jobs=2
>   make --quiet test
> -'
> +"

This interpolates $cache_dir while generating the snippet. You've stuck
it in single-quotes, which prevents most quoting problems, but obviously
it's an issue if the variable itself contains a single-quote. Is there
a reason not to just export $cache_dir in the environment and access it
directly from the snippet?

Probably not a _big_ deal, since we control the contents of the
variable, but it just seems like a fragile practice to avoid.

-Peff

Re: [PATCH 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job

[Jeff King]

On Mon, Jan 22, 2018 at 02:32:17PM +0100, SZEDER Gábor wrote:

> All 'ci/*' scripts use 'set -e' to break the build job if a command
> fails, except 'ci/run-linux32-build.sh' which relies on the && chain
> to do the same.  This inconsistency among the 'ci/*' scripts is asking
> for trouble: I forgot about the && chain more than once while working
> on this patch series.

I think this actually fixes a bug:

>  # Update packages to the latest available versions
>  linux32 --32bit i386 sh -c '
>  apt update >/dev/null &&
>  apt install -y build-essential libcurl4-openssl-dev libssl-dev \
>   libexpat-dev gettext python >/dev/null
> -' &&
> +'

If this step failed, then...

>  # If this script runs inside a docker container, then all commands are
>  # usually executed as root. Consequently, the host user might not be
>  # able to access the test output files.
>  # If a host user id is given, then create a user "ci" with the host user
>  # id to make everything accessible to the host user.
> -HOST_UID=$1 &&
> -CI_USER=$USER &&
> -test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER) &&

We'd have triggered the right-hand side of this "||", and run the rest
of the script.  The whole "||" block should have been inside {}.

But after your patch, it should be fine with "set -e". Although...

> +HOST_UID=$1
> +CI_USER=$USER
> +test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER)

If this "useradd" step fails, we wouldn't abort the script, because it's
part of a conditional. You'd need a manual "|| exit 1" at the end of
this line. Or to use a real "if" block.

Reading this line, I'm also slightly confused by setting CI_USER in the
subshell, and then only using it once. Should it be respecting the outer
CI_USER setting? If not, should it just hard-code "ci" on the useradd
command-line?  But that has nothing to do with your patch here.

-Peff

Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute

[Jeff King]

On Tue, Jan 23, 2018 at 11:25:58AM +0100, Simon Ruderich wrote:

> On Mon, Jan 22, 2018 at 07:54:01PM -0500, Jeff King wrote:
> > But anyway, that was a bit of a tangent. Certainly the smaller change is
> > just standardizing on sizeof(*foo), which I think most people agree on
> > at this point. It might be worth putting in CodingGuidelines.
> 
> Personally I prefer sizeof(*foo) which is a well non-idiom, used
> in many projects and IMHO easy to read and understand.

Me too.

> I've played a little with coccinelle and the following spatch
> seems to catch many occurrences of sizeof(struct ..) (the first
> hunk seems to expand multiple times causing conflicts in the
> generated patch). Cases like a->b = xcalloc() are not matched, I
> don't know enough coccinelle for that. If there's interest I
> could prepare patches, but it will create quite some code churn.

Yeah, I'm not sure what our current policy is there. Traditionally our
strategy was not to churn code, but to update old idioms as they were
touched. Especially if the change was not urgent, but mostly stylistic
(which this one is).

But with Coccinelle, it's a lot easier to apply the change tree-wide, and
to convert topics in flight as they get merged. The maintainer still
gets conflicts with topics-in-flight that touch converted areas, though.
So I'd be curious to hear if Junio's opinion has changed at all.

-Peff

Re: [PATCH] enable core.fsyncObjectFiles by default

[Jeff King]

On Tue, Jan 23, 2018 at 12:45:53AM -0500, Theodore Ts'o wrote:

> What I was thinking about instead is that in cases where we know we
> are likely to be creating a large number of loose objects (whether
> they referenced or not), in a world where we will be calling fsync(2)
> after every single loose object being created, pack files start
> looking *way* more efficient.  So in general, if you know you will be
> creating N loose objects, where N is probably around 50 or so, you'll
> want to create a pack instead.
> 
> One of those cases is "repack -A", and in that case the loose objects
> are all going tobe not referenced, so it would be a "cruft pack".  But
> in many other cases where we might be importing from another DCVS,
> which will be another case where doing an fsync(2) after every loose
> object creation (and where I have sometimes seen it create them *all*
> loose, and not use a pack at all), is going to get extremely slow and
> painful.

Ah, I see. I think in the general case of git operations this is hard
(because most object writes don't realize the larger operation that
they're a part of). But I agree that those two are the low-hanging fruit
(imports should already be using fast-import, and "cruft packs" are not
too hard an idea to implement).

I agree that a cruft-pack implementation could just be for "repack -A",
and does not have to collect otherwise loose objects. I think part of my
confusion was that you and I are coming to the idea from different
angles: you care about minimizing fsyncs, and I'm interested in stopping
the problem where you have too many loose objects after running auto-gc.
So I care more about collecting those loose objects for that case.

> > So if we pack all the loose objects into a cruft pack, the mtime of the
> > cruft pack becomes the new gauge for "recent". And if we migrate objects
> > from old cruft pack to new cruft pack at each gc, then they'll keep
> > getting their mtimes refreshed, and we'll never drop them.
> 
> Well, I was assuming that gc would be a special case which doesn't the
> mtime of the old cruft pack.  (Or more generally, any time an object
> is gets copied out of the cruft pack, either to a loose object, or to
> another pack, the mtime on the source pack should not be touched.)

Right, that's the "you have multiple cruft packs" idea which has been
discussed[1] (each one just hangs around until its mtime expires, and
may duplicate objects found elsewhere).

That does end up with one pack per gc, which just punts the "too many
loose objects" to "too many packs". But unless the number of gc runs you
do is very high compared to the expiration time, we can probably ignore
that.

-Peff

[1] 
https://public-inbox.org/git/20170610080626.sjujpmgkli4mu...@sigill.intra.peff.net/

Charity Funds

[Alice Walton]

my name is Mrs. Alice Walton, a business woman an America Citizen and the 
heiress to the fortune of Walmart stores, born October 7, 1949. I have a 
mission for you worth $100,000,000.00(Hundred Million United State Dollars) 
which I intend using for CHARITY

Re: git merge-tree: bug report and some feature requests

[Edward Thomson]

On Tue, Jan 23, 2018 at 7:08 AM, Josh Bleecher Snyder
 wrote:
> Looking over your list above, at a minimum, libgit2 might not have a
> particularly good way to represent submodule/file or
> submodule/directory conflicts, because is-a-submodule is defined
> external to a git_index_entry.

libgit2 should detect submodule/file and submodule/directory
conflicts.

While, yes, some metadata about the submodule is located outside the
index, you can look at the mode to determine that this _is_ a
submodule.  You should be able to reliably detect submodule/file
conflicts because one will be a regular or executable file (mode
0100644 or 0100755), while the other entry at the same path will
be a gitlink (mode 016).

Similarly, submodule/directory conflict detection works just like
regular file/directory conflict detection.  If some path `foo` is
a submodule (or a regular file) then some path `foo/bar` existing
in the other side of the merge causes a conflict.

> Cataloging or special-casing all possible conflict types does seem
> unwise because of the sheer number of kinds of conflicts.
>
> But the alternative appears to be punting entirely, as libgit2 does,
> and merely providing something akin to three index entries.

Indeed, when I added merge to libgit2, we put the higher-level conflict
analysis into application code because there was not much interest in it
at the time.  I've been meaning to add this to `git_status` in libgit2,
but it's not been a high priority.

> This which
> leaves it unclear what exactly the conflict was, at which point any
> user (read: porcelain developer) will end up having to recreate some
> merge logic to figure out what went wrong. And if merge-tree starts
> doing rename detection, the user might then have to emulate that as
> well.

That's not a good idea.  Trying to figure out what merge did would be
painful at best, and likely impossible, since a rename conflict is
recorded in the main index without any way to piece it together.  eg:

100644 deadbeefdeadbeefdeadbeefdeadbeefdeadbeef 1   bar.c
100644 cafec4fecafec4fecafec4fecafec4fecafec4fe 2   bar.c
100644 c4cc188a892898e13927dc4a02e7f68814b874b2 1   foo.c
100644 71f5af150b25e3d2d67ff46759311401036f 2   foo.c
100644 351cfbdd55d656edd2c5c995aae3caafb9ec11fa 3   rename1.c
100644 e407c7d138fb457674c3b114fcf47748169ab0c5 3   rename2.c

This is the main index that results when bar.c has a rename/edit
conflict, and foo.c also has a rename/edit conflict.  One was renamed
to rename1.c and the other to rename2.c.  Trying to determine which is
which _after the fact_ would be regrettable.  Especially since rename
detection is not static - you would need to know the thresholds that
were configured at the time merge was performed and try to replay
the rename detection with that.

libgit2 records a `NAME` section in the index that pairs the rename
detection decisions that it performed so that you can analyze them
and display them after the fact.

-ed

Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute

[Simon Ruderich]

On Mon, Jan 22, 2018 at 07:54:01PM -0500, Jeff King wrote:
> But anyway, that was a bit of a tangent. Certainly the smaller change is
> just standardizing on sizeof(*foo), which I think most people agree on
> at this point. It might be worth putting in CodingGuidelines.

Personally I prefer sizeof(*foo) which is a well non-idiom, used
in many projects and IMHO easy to read and understand.

I've played a little with coccinelle and the following spatch
seems to catch many occurrences of sizeof(struct ..) (the first
hunk seems to expand multiple times causing conflicts in the
generated patch). Cases like a->b = xcalloc() are not matched, I
don't know enough coccinelle for that. If there's interest I
could prepare patches, but it will create quite some code churn.

Regards
Simon

@@
type T;
identifier x;
@@
- T *x = xmalloc(sizeof(T));
+ T *x = xmalloc(sizeof(*x));

@@
type T;
T *x;
@@
- x = xmalloc(sizeof(T));
+ x = xmalloc(sizeof(*x));


@@
type T;
identifier x;
expression n;
@@
- T *x = xcalloc(n, sizeof(T));
+ T *x = xcalloc(n, sizeof(*x));

@@
type T;
T *x;
expression n;
@@
- x = xcalloc(n, sizeof(T));
+ x = xcalloc(n, sizeof(*x));


@@
type T;
T x;
@@
- memset(, 0, sizeof(T));
+ memset(, 0, sizeof(x));

@@
type T;
T *x;
@@
- memset(x, 0, sizeof(T));
+ memset(x, 0, sizeof(*x));

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[PATCH v2 2/2] sequencer: run 'prepare-commit-msg' hook

[Phillip Wood]

From: Phillip Wood 

Commit 356ee4659b ("sequencer: try to commit without forking 'git
commit'", 2017-11-24) forgot to run the 'prepare-commit-msg' hook when
creating the commit. Fix this by writing the commit message to a
different file and running the hook. Using a different file means that
if the commit is cancelled the original message file is
unchanged. Also move the checks for an empty commit so the order
matches 'git commit'.

Reported-by: Dmitry Torokhov 
Signed-off-by: Phillip Wood 
Reviewed-by: Ramsay Jones 
---

Notes:
Changes since v1:
  - marked run_prepare_commit_msg_hook() as static (Thanks to Ramsey Jones)

 builtin/commit.c   |  2 --
 sequencer.c| 69 +++---
 sequencer.h|  1 +
 t/t7505-prepare-commit-msg-hook.sh |  8 ++---
 4 files changed, 61 insertions(+), 19 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 
4a64428ca8ed8c3066aec7cfd8ad7b71217af7dd..5dd766af2842dddb80d30cd73b8be8ccb4956eac
 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -66,8 +66,6 @@ N_("If you wish to skip this commit, use:\n"
 "Then \"git cherry-pick --continue\" will resume cherry-picking\n"
 "the remaining commits.\n");
 
-static GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
-
 static const char *use_message_buffer;
 static struct lock_file index_lock; /* real index */
 static struct lock_file false_lock; /* used only for partial commits */
diff --git a/sequencer.c b/sequencer.c
index 
63a8ec9a61e7a7bf603ffa494621af79b60e0b76..5bfdc4044233d5f809f9f1fbc55ebe3da477e3f0
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -29,6 +29,8 @@
 const char sign_off_header[] = "Signed-off-by: ";
 static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
+GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
+
 GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
 
 static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
@@ -891,6 +893,31 @@ void commit_post_rewrite(const struct commit *old_head,
run_rewrite_hook(_head->object.oid, new_head);
 }
 
+static int run_prepare_commit_msg_hook(struct strbuf *msg, const char *commit)
+{
+   struct argv_array hook_env = ARGV_ARRAY_INIT;
+   int ret;
+   const char *name;
+
+   name = git_path_commit_editmsg();
+   if (write_message(msg->buf, msg->len, name, 0))
+   return -1;
+
+   argv_array_pushf(_env, "GIT_INDEX_FILE=%s", get_index_file());
+   argv_array_push(_env, "GIT_EDITOR=:");
+   if (commit)
+   ret = run_hook_le(hook_env.argv, "prepare-commit-msg", name,
+ "commit", commit, NULL);
+   else
+   ret = run_hook_le(hook_env.argv, "prepare-commit-msg", name,
+ "message", NULL);
+   if (ret)
+   ret = error(_("'prepare-commit-msg' hook failed"));
+   argv_array_clear(_env);
+
+   return ret;
+}
+
 static const char implicit_ident_advice_noconfig[] =
 N_("Your name and email address were configured automatically based\n"
 "on your username and hostname. Please check that they are accurate.\n"
@@ -1051,8 +1078,9 @@ static int try_to_commit(struct strbuf *msg, const char 
*author,
struct commit_list *parents = NULL;
struct commit_extra_header *extra = NULL;
struct strbuf err = STRBUF_INIT;
-   struct strbuf amend_msg = STRBUF_INIT;
+   struct strbuf commit_msg = STRBUF_INIT;
char *amend_author = NULL;
+   const char *hook_commit = NULL;
enum commit_msg_cleanup_mode cleanup;
int res = 0;
 
@@ -1069,8 +1097,9 @@ static int try_to_commit(struct strbuf *msg, const char 
*author,
const char *orig_message = NULL;
 
find_commit_subject(message, _message);
-   msg = _msg;
+   msg = _msg;
strbuf_addstr(msg, orig_message);
+   hook_commit = "HEAD";
}
author = amend_author = get_author(message);
unuse_commit_buffer(current_head, message);
@@ -1084,16 +1113,6 @@ static int try_to_commit(struct strbuf *msg, const char 
*author,
commit_list_insert(current_head, );
}
 
-   cleanup = (flags & CLEANUP_MSG) ? COMMIT_MSG_CLEANUP_ALL :
- opts->default_msg_cleanup;
-
-   if (cleanup != COMMIT_MSG_CLEANUP_NONE)
-   strbuf_stripspace(msg, cleanup == COMMIT_MSG_CLEANUP_ALL);
-   if (!opts->allow_empty_message && message_is_empty(msg, cleanup)) {
-   res = 1; /* run 'git commit' to display error message */
-   goto out;
-   }
-
if (write_cache_as_tree(tree.hash, 0, NULL)) {
res = error(_("git write-tree failed 

[PATCH v2 0/2] sequencer: run 'prepare-commit-msg' hook​

[Phillip Wood]

From: Phillip Wood 

I've updated the patches in response to comments, there are just a
couple of small changes. Thanks to Ramsay and Eric for their reviews.

Best Wishes

Phillip

Original cover letter:

These two patches add some tests and fix the sequencer to run the
'prepare-commit-msg' hook when committing without forking 'git commit'


Phillip Wood (2):
  t7505: Add tests for cherry-pick and rebase -i/-p
  sequencer: run 'prepare-commit-msg' hook

 builtin/commit.c   |   2 -
 sequencer.c|  69 
 sequencer.h|   1 +
 t/t7505-prepare-commit-msg-hook.sh | 127 +++--
 t/t7505/expected-rebase-i  |  17 +
 t/t7505/expected-rebase-p  |  18 ++
 6 files changed, 215 insertions(+), 19 deletions(-)
 create mode 100644 t/t7505/expected-rebase-i
 create mode 100644 t/t7505/expected-rebase-p

-- 
2.15.1

[PATCH v2 1/2] t7505: Add tests for cherry-pick and rebase -i/-p

[Phillip Wood]

From: Phillip Wood 

Check that cherry-pick and rebase call the 'prepare-commit-msg' hook
correctly. The expected values for the hook arguments are taken to
match the current master branch. I think there is scope for improving
the arguments passed so they make a bit more sense - for instance
cherry-pick currently passes different arguments depending on whether
the commit message is being edited. Also the arguments for rebase
could be improved. Commit 7c4188360ac ("rebase -i: proper
prepare-commit-msg hook argument when squashing", 2008-10-3) apparently
changed things so that when squashing rebase would pass 'squash' as
the argument to the hook but that has been lost.

I think that it would make more sense to pass 'message' for revert and
cherry-pick -x/-s (i.e. cases where there is a new message or the
current message in modified by the command), 'squash' when squashing
with a new message and 'commit HEAD/CHERRY_PICK_HEAD'
otherwise (picking and squashing without a new message).

Signed-off-by: Phillip Wood 
Reviewed-by: Eric Sunshine 
---

Notes:
Changes since v1
  - use test_seq for portability (Thanks to Eric Sunshine)

 t/t7505-prepare-commit-msg-hook.sh | 127 +++--
 t/t7505/expected-rebase-i  |  17 +
 t/t7505/expected-rebase-p  |  18 ++
 3 files changed, 158 insertions(+), 4 deletions(-)
 create mode 100644 t/t7505/expected-rebase-i
 create mode 100644 t/t7505/expected-rebase-p

diff --git a/t/t7505-prepare-commit-msg-hook.sh 
b/t/t7505-prepare-commit-msg-hook.sh
index 
b13f72975ecce17887c4c8275c6935d78d4b09a0..a3dd3545030c2a29a1ca4502f26b412f92f0375a
 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -4,6 +4,38 @@ test_description='prepare-commit-msg hook'
 
 . ./test-lib.sh
 
+test_expect_success 'set up commits for rebasing' '
+   test_commit root &&
+   test_commit a a a &&
+   test_commit b b b &&
+   git checkout -b rebase-me root &&
+   test_commit rebase-a a aa &&
+   test_commit rebase-b b bb &&
+   for i in $(test_seq 1 13)
+   do
+   test_commit rebase-$i c $i
+   done &&
+   git checkout master &&
+
+   cat >rebase-todo <<-EOF
+   pick $(git rev-parse rebase-a)
+   pick $(git rev-parse rebase-b)
+   fixup $(git rev-parse rebase-1)
+   fixup $(git rev-parse rebase-2)
+   pick $(git rev-parse rebase-3)
+   fixup $(git rev-parse rebase-4)
+   squash $(git rev-parse rebase-5)
+   reword $(git rev-parse rebase-6)
+   squash $(git rev-parse rebase-7)
+   fixup $(git rev-parse rebase-8)
+   fixup $(git rev-parse rebase-9)
+   edit $(git rev-parse rebase-10)
+   squash $(git rev-parse rebase-11)
+   squash $(git rev-parse rebase-12)
+   edit $(git rev-parse rebase-13)
+   EOF
+'
+
 test_expect_success 'with no hook' '
 
echo "foo" > file &&
@@ -31,17 +63,40 @@ mkdir -p "$HOOKDIR"
 echo "#!$SHELL_PATH" > "$HOOK"
 cat >> "$HOOK" <<'EOF'
 
+GIT_DIR=$(git rev-parse --git-dir)
+if test -d "$GIT_DIR/rebase-merge"
+then
+  rebasing=1
+else
+  rebasing=0
+fi
+
+get_last_cmd () {
+  tail -n1 "$GIT_DIR/rebase-merge/done" | {
+read cmd id _
+git log --pretty="[$cmd %s]" -n1 $id
+  }
+}
+
 if test "$2" = commit; then
-  source=$(git rev-parse "$3")
+  if test $rebasing = 1
+  then
+source="$3"
+  else
+source=$(git rev-parse "$3")
+  fi
 else
   source=${2-default}
 fi
-if test "$GIT_EDITOR" = :; then
-  sed -e "1s/.*/$source (no editor)/" "$1" > msg.tmp
+test "$GIT_EDITOR" = : && source="$source (no editor)"
+
+if test $rebasing = 1
+then
+  echo "$source $(get_last_cmd)" >"$1"
 else
   sed -e "1s/.*/$source/" "$1" > msg.tmp
+  mv msg.tmp "$1"
 fi
-mv msg.tmp "$1"
 exit 0
 EOF
 chmod +x "$HOOK"
@@ -156,6 +211,63 @@ test_expect_success 'with hook and editor (merge)' '
test "$(git log -1 --pretty=format:%s)" = "merge"
 '
 
+test_rebase () {
+   expect=$1 &&
+   mode=$2 &&
+   test_expect_$expect C_LOCALE_OUTPUT "with hook (rebase $mode)" '
+   test_when_finished "\
+   git rebase --abort
+   git checkout -f master
+   git branch -D tmp" &&
+   git checkout -b tmp rebase-me &&
+   GIT_SEQUENCE_EDITOR="cp rebase-todo" &&
+   GIT_EDITOR="\"$FAKE_EDITOR\"" &&
+   (
+   export GIT_SEQUENCE_EDITOR GIT_EDITOR &&
+   test_must_fail git rebase $mode b &&
+   echo x>a &&
+   git add a &&
+   test_must_fail git rebase --continue &&
+   echo x>b &&
+   git add b &&
+   git commit &&
+   git rebase --continue &&
+   echo y>a &&
+   git add a &&

[PATCH v2] SQUASH convert: add tracing for 'working-tree-encoding' attribute

[lars . schneider]

From: Lars Schneider 

Hi Junio,

I overlooked a typo pointed out in Simon's review. Here is a new patch
for squashing. Sorry for the trouble!

@Eric: Thanks for spotting this!

Cheers,
Lars


 convert.c| 8 ++--
 t/t0028-working-tree-encoding.sh | 2 ++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/convert.c b/convert.c
index 13fad490ce..dce2f6e201 100644
--- a/convert.c
+++ b/convert.c
@@ -1165,8 +1165,12 @@ static struct encoding *git_path_check_encoding(struct 
attr_check_item *check)
if (!strcasecmp(value, default_encoding))
return NULL;

-   enc = xcalloc(1, sizeof(struct convert_driver));
-   enc->name = xstrdup_toupper(value);  /* aways use upper case names! */
+   enc = xcalloc(1, sizeof(*enc));
+   /*
+* Ensure encoding names are always upper case (e.g. UTF-8) to
+* simplify subsequent string comparisons.
+*/
+   enc->name = xstrdup_toupper(value);
*encoding_tail = enc;
encoding_tail = &(enc->next);

diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh
index 0f36d4990a..a6da61280d 100755
--- a/t/t0028-working-tree-encoding.sh
+++ b/t/t0028-working-tree-encoding.sh
@@ -22,6 +22,8 @@ test_expect_success 'setup test repo' '
 test_expect_success 'ensure UTF-8 is stored in Git' '
git cat-file -p :test.utf16 >test.utf16.git &&
test_cmp_bin test.utf8.raw test.utf16.git &&
+
+   # cleanup
rm test.utf8.raw test.utf16.git
 '


base-commit: 21f4dac5aba07a6109285c57a0478bf502e09009
--
2.16.0

Re: [PATCH/RFC 0/2] Automate updating git-completion.bash a bit

[Duy Nguyen]

On Mon, Jan 22, 2018 at 07:03:24PM +0100, SZEDER Gábor wrote:
> On Wed, Jan 17, 2018 at 10:34 AM, Duy Nguyen  wrote:
> > Actually I forgot another option. What if we automate updating the
> > script at "compile" time instead of calling git at run time? E.g. with
> > something like below, a contributor could just run
> >
> > make update-completion
> >
> > then add git-completion.bash changes to the same patch that introduces
> > new options. If they forget
> 
> They inevitably will :)

OK. We go with the first option then (with caching to reduce overhead
on Windows). I'm not going to bother you with actual code changes. The
diff of completable options looks like below. It would be great if
people could check if some options should NOT be completable for some
reason.

A couple points

- Ignore options that are removed in the diff. They are removed for a
  reason which I will explain when real patches come.

- There are regressions where --foo= becomes --foo, I hope this is ok
  for now. We can tweak this later.

- In some commands you can complete both --foo and --foo= (now it's
  just one form, --foo). I would argue that it's pointless. It's no
  big deal to type '=' (or a space) after we have completed --foo.

- I feel --force is not treated the same way everywhere. But I guess
  that's ok since "forcing" in some command context may be less severe
  than others.

-- 8< --
diff --git a/git-add.txt b/git-add.txt
index 1c249a3..2693cc1 100644
--- a/git-add.txt
+++ b/git-add.txt
@@ -1,10 +1,15 @@
+--all
 --chmod=
 --dry-run
 --edit
 --force
 --ignore-errors
+--ignore-missing
+--ignore-removal
 --intent-to-add
 --interactive
 --patch
 --refresh
+--renormalize
 --update
+--verbose
diff --git a/git-am.txt b/git-am.txt
index b0082bf..552dc96 100644
--- a/git-am.txt
+++ b/git-am.txt
@@ -1,12 +1,24 @@
 --3way
 --committer-date-is-author-date
+--directory
+--exclude
+--gpg-sign
 --ignore-date
 --ignore-space-change
 --ignore-whitespace
+--include
 --interactive
 --keep
+--keep-cr
+--keep-non-patch
+--message-id
+--no-keep-cr
 --no-utf8
+--patch-format
+--quiet
+--reject
+--resolvemsg=
 --scissors
 --signoff
 --utf8
---whitespace=
+--whitespace
diff --git a/git-apply.txt b/git-apply.txt
index 6bf4c2f..71d53d2 100644
--- a/git-apply.txt
+++ b/git-apply.txt
@@ -1,13 +1,17 @@
+--3way
+--allow-overlap
 --apply
+--build-fake-ancestor=
 --cached
 --check
---directory=
---exclude=
+--directory
+--exclude
 --ignore-space-change
 --ignore-whitespace
 --inaccurate-eof
+--include
 --index
---index-info
+--no-add
 --no-add
 --numstat
 --recount
@@ -17,4 +21,4 @@
 --summary
 --unidiff-zero
 --verbose
---whitespace=
+--whitespace
diff --git a/git-branch.txt b/git-branch.txt
index 69594e3..9d308aa 100644
--- a/git-branch.txt
+++ b/git-branch.txt
@@ -1,10 +1,14 @@
---abbrev=
+--abbrev
+--all
 --color
 --column
 --contains
 --copy
+--create-reflog
 --delete
 --edit-description
+--format=
+--ignore-case
 --list
 --merged
 --move
@@ -15,9 +19,10 @@
 --no-merged
 --no-track
 --points-at
+--quiet
 --remotes
 --set-upstream-to=
---sort=
+--sort
 --track
 --unset-upstream
 --verbose
diff --git a/git-checkout.txt b/git-checkout.txt
index 493a1fe..75f19d2 100644
--- a/git-checkout.txt
+++ b/git-checkout.txt
@@ -1,12 +1,14 @@
 --conflict=
 --detach
+--ignore-other-worktrees
 --ignore-skip-worktree-bits
 --merge
 --no-recurse-submodules
 --no-track
---orphan
+--orphan=
 --ours
 --patch
+--progress
 --quiet
 --recurse-submodules
 --theirs
diff --git a/git-cherry-pick.txt b/git-cherry-pick.txt
index 39ba895..f8cdbce 100644
--- a/git-cherry-pick.txt
+++ b/git-cherry-pick.txt
@@ -1,5 +1,14 @@
+--abort
+--allow-empty
+--allow-empty-message
+--continue
 --edit
+--ff
+--gpg-sign
+--keep-redundant-commits
 --mainline
 --no-commit
+--quit
 --signoff
 --strategy=
+--strategy-option
diff --git a/git-clean.txt b/git-clean.txt
index 40407f7..10c6155 100644
--- a/git-clean.txt
+++ b/git-clean.txt
@@ -1,2 +1,4 @@
 --dry-run
+--exclude
+--interactive
 --quiet
diff --git a/git-clone.txt b/git-clone.txt
index f6e892b..1b6a4da 100644
--- a/git-clone.txt
+++ b/git-clone.txt
@@ -1,18 +1,29 @@
 --bare
---branch
---depth
+--branch=
+--config
+--depth=
+--dissociate
+--ipv4
+--ipv6
+--jobs=
 --local
 --mirror
 --no-checkout
 --no-hardlinks
 --no-single-branch
 --no-tags
---origin
+--origin=
+--progress
 --quiet
 --recurse-submodules
 --reference
+--reference-if-able
+--separate-git-dir=
+--shallow-exclude
+--shallow-since=
 --shallow-submodules
 --shared
 --single-branch
 --template=
---upload-pack
+--upload-pack=
+--verbose
diff --git a/git-commit.txt b/git-commit.txt
index 2f98a59..337a57e 100644
--- a/git-commit.txt
+++ b/git-commit.txt
@@ -1,20 +1,26 @@
 --all
---allow-empty
 --amend
 --author=
+--branch
 --cleanup=
---date
+--date=
 --dry-run
 --edit
 --file=
 --fixup=
+--gpg-sign
 --include
 --interactive
---message=
+--long
+--message
 --no-edit
+--no-post-rewrite
 --no-verify
+--no-verify
+--null
 --only
 --patch
+--porcelain
 --quiet
 

Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute

[Simon Ruderich]

On Mon, Jan 22, 2018 at 01:35:25PM +0100, Lars Schneider wrote:
>>> +   enc->name = xstrdup_toupper(value);  /* aways use upper case names! */
>>
>> "aways" -> "always" and I think the comment should say why
>> uppercase is important.
>
> Would that be better?
>
>   /* Aways use upper case names to simplify subsequent string comparison. 
> */
>   enc->name = xstrdup_toupper(value);
>
> AFAIK uppercase and lowercase names are both valid. I just wanted to
> ensure that we use one consistent casing. That reads better in error messages
> and I don't need to check for the letter case in has_prohibited_utf_bom()
> and friends in utf8.c

Sounds good (minus the "Aways" typo Eric already noticed).

>> Micro-nit: For consistency with the previous test, remove the
>> empty line and comment (or just keep the files generated from the
>> "setup test repo" phase and don't explicitly delete them)?
>
> I would rather add a new line and a comment to the previous test
> to be consistent.
>
> I know we could leave the files but these lingering files could
> always surprise writers of future tests (at least they surprised
> me in other tests).

Sure, that sounds good. Just noticed the inconsistency and wanted
to mention it.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9