[PATCH] fsck: fix leak when traversing trees

[Eric Wong]

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")

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)
-- 
EW

RE: [RFE/RFC] format-patch/diff via path

[Randall S. Becker]

On January 20, 2018 2:15 AM, Junio C Hamano wrote:
> "Randall S. Becker"  writes:
> 
> > I’m still a bit perplexed by some behaviour seen today, and am looking
> > for a clean way to deal with it that the documentation does not make
> > clear. So, I’m asking in a different way. Suppose a graph of
> >
> > A---B---C---D---E
> > \  \   /
> >   \F—G/
> >
> 
> An ASCII art that is not drawn for fixed-width font is by definition
> understandable only by the person who drew it X-<.  I am guessing that F is a
> child of both A and B (but I am not sure, as I do not see a reason why it
> should even be a merge to begin with, so my guess is likely to be wrong), and
> E is a merge between D and G.

My bad... outlook... and user. 
> 
> IOW, I am guessing that the below is the equivalent of what you drew for
> those who look at the picture in fixed-width font:
> 
> A---B---C---D---E
>  \   \ /
>   .---F---G

As unintelligible, X-<, but you are probably correct.

> > When trying to perform a format-patch from B to E, I was seeing
> > commits B-A-F-G-E rather than what I wanted B-C-D-E.
> 
> Assuming that E is a merge, format-patch output should not show E anyway
> (i.e. think in terms of "git log --no-merges --reverse", instead of fearing 
> that
> format-patch is somehow more magical---it is not).  So if you want to show
> the comit B, C and D (meaning three patches, i.e. "diff A B", "diff B C", and
> "diff C D"), then you would do "format-patch A..D", not "format-patch A..E".
> If you meant that you are not interested in the change between A and B,
> then the range would be "B..D" instead of "A..D".  Ending the range at "E"
> means you want to see what is reachable from E, and unless you say you are
> not interested in G, you would get G, if you only say you are not interested 
> in
> A (or B), as G is not reachable from A (or B).

While the end point, E was the same regardless of which path, I was interested 
in submitting the patches along B..E. A is the parent of B and F and was 
included in the format-patch, which then forward through F and G then E.

> It is unclear how you told format-patch when "trying to perform a format-
> patch from B to E" from your description, but if you said "format-patch
> A^..E", it is likely that you would have seen all commits in the depicted part
> of the graph except for merge commits.

That seems to be the case. I used format-patch B..E with no other args. A was 
not specified but got drawn in. D-E was a merge so is that why that path wasn't 
selected? I'd still like to be able to include merges - is that a dream?

Re: [RFE/RFC] format-patch/diff via path

[Junio C Hamano]

"Randall S. Becker"  writes:

> I’m still a bit perplexed by some behaviour seen today, and am looking for a 
> clean way to deal with it that the documentation does not make clear. So, I’m 
> asking in a different way. Suppose a graph of
>
> A---B---C---D---E
> \  \   /
>   \F—G/
>

An ASCII art that is not drawn for fixed-width font is by definition
understandable only by the person who drew it X-<.  I am guessing
that F is a child of both A and B (but I am not sure, as I do not
see a reason why it should even be a merge to begin with, so my
guess is likely to be wrong), and E is a merge between D and G.

IOW, I am guessing that the below is the equivalent of what you drew
for those who look at the picture in fixed-width font:

A---B---C---D---E
 \   \ /
  .---F---G

> When trying to perform a format-patch from B to E, I was seeing
> commits B-A-F-G-E rather than what I wanted B-C-D-E.

Assuming that E is a merge, format-patch output should not show E
anyway (i.e. think in terms of "git log --no-merges --reverse",
instead of fearing that format-patch is somehow more magical---it is
not).  So if you want to show the comit B, C and D (meaning three
patches, i.e. "diff A B", "diff B C", and "diff C D"), then you
would do "format-patch A..D", not "format-patch A..E".  If you meant
that you are not interested in the change between A and B, then the
range would be "B..D" instead of "A..D".  Ending the range at "E"
means you want to see what is reachable from E, and unless you say
you are not interested in G, you would get G, if you only say you
are not interested in A (or B), as G is not reachable from A (or B).

It is unclear how you told format-patch when "trying to perform a
format-patch from B to E" from your description, but if you said
"format-patch A^..E", it is likely that you would have seen all
commits in the depicted part of the graph except for merge commits.

Re: [PATCH] repository: pre-initialize hash algo pointer

[Junio C Hamano]

Junio C Hamano  writes:

> Eric Sunshine  writes:
>
>>> I'm still quite mystified as to why this is working on Linux and not
>>> macOS, but I can only guess that compilers are just very advanced and
>>> have somehow concluded that we would clearly never dereference a NULL
>>> pointer, so they picked the only non-NULL value.
>>
>> Now that we know (due to Duy's excellent detective work[1]) that the
>> trigger is files with names differing only in case on case-insensitive
>> filesystems, the commit message can be updated appropriately.
>
> Thanks.  Let me apply the following and do a 2.16.1, hopefully by
> the end of day or mid tomorrow at the latest.  Test to protect the
> fix can come as a separate follow-up patch.

I actually changed my mind and decided not to rush 2.16.1 with just
this change.  After all, even though it is better not to crash in
such a problematic case, a "successful" clone of such a project on
such a filesystem cannot be sanely used *anyway*, so in that sense
the "fix" would stop "clone" from segfaulting but the resulting
repository would still be "wrong" (e.g. "git status" immediately
after clone would likely to say that the working tree is already
dirty and missing one of the two paths, or something silly like
that).

[RFE/RFC] format-patch/diff via path

[Randall S. Becker]

I’m still a bit perplexed by some behaviour seen today, and am looking for a 
clean way to deal with it that the documentation does not make clear. So, I’m 
asking in a different way. Suppose a graph of

A---B---C---D---E
\  \   /
  \F—G/

When trying to perform a format-patch from B to E, I was seeing commits 
B-A-F-G-E rather than what I wanted B-C-D-E.  F and G were younger commits than 
C and D, which I assume (very likely wrongly) is why diff was giving 
preferential treatment to that path.

What I am trying to figure out is whether there is a clean way to force 
format-patch along the B-C-D-E path. If not, would it be worth starting up a 
small project to make this possible (not knowing exactly where to start), but I 
would envision something like: git format-patch –via=C B..E

I may be just missing something obvious (new to format-patch operations 
myself). 

Cheers,
Randall
P.S. Bad ideas happen when tests run for a long time 


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

regression in output of git-pull --rebase --recurse-submodules=yes --quiet

[Robin H. Johnson]

Somewhere between 2.13.6 & 2.14.1 there's an output regression. I
haven't done a bisect to trace it down further yet.

Specifically, --rebase --recurse-submodules=yes seems to cause --quiet
to not be effective anymore.

Full commandline:
$ git pull --rebase --recurse-submodules --quiet

In 2.13.6, there is no output, it's quiet as expect.

In 2.14.1, you get:
HEAD is up to date.
Submodule path '_data/news': rebased into 
'a50b763c338161b4621d23e9fa5cd6e11455d6ca'
HEAD is up to date.
Submodule path 'glep': rebased into 'e1f100ec3ba44ab1672d61cabf4690b355e46158'

Steps to reproduction:
1. git clone --recurse-submodules \
 https://anongit.gentoo.org/git/sites/www.git
2. cd www
3. git submodule foreach --quiet git pull --quiet origin master
4. git pull --rebase --recurse-submodules=yes --quiet

Repeat step 4 for repeated bug output.
If you drop the --rebase, then you need to re-run step 3 first.

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robb...@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136


signature.asc
Description: Digital signature

Re: [PATCH 8/8] rebase -i: introduce --recreate-merges=no-rebase-cousins

[Eric Sunshine]

On Thu, Jan 18, 2018 at 10:36 AM, Johannes Schindelin
 wrote:
> This one is a bit tricky to explain, so let's try with a diagram:
> [...]
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> @@ -57,8 +59,13 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
> +   if (no_rebase_cousins >= 0&& !recreate_merges)

Style: space before &&

> +   warning(_("--[no-]rebase-cousins has no effect without "
> + "--recreate-merges"));

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

[Eric Wong]

Gargi Sharma  wrote:
> --- a/list.h
> +++ b/list.h
> @@ -93,6 +93,13 @@ static inline void list_move(struct list_head *elem, 
> struct list_head *head)
>   list_add(elem, head);
>  }
>  
> +/* Move to the front of the list. */
> +static inline void list_move_to_front(struct list_head *elem, struct 
> list_head *head)
> +{
> + list_del(elem);
> + list_add(elem, head);
> +}
> +

Since we already have list_move and it does the same thing,
I don't think we need a new function, here.

Hackers coming from other projects (glibc/urcu/Linux kernel)
might appreciate having fewer differences from what they're used
to.

Anyways thanks for working on this!

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

[Eric Sunshine]

On Fri, Jan 19, 2018 at 6:36 PM, 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.

A couple minor style nits below... (may not be worth a re-roll)

> Signed-off-by: Gargi Sharma 
> ---
> diff --git a/cache.h b/cache.h
> @@ -1587,10 +1588,10 @@ 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;
> +

Unnecessary extra blank line.

>  struct pack_entry {
> off_t offset;
> diff --git a/packfile.c b/packfile.c
> @@ -859,9 +859,9 @@ static void prepare_packed_git_mru(void)
>  {
> struct packed_git *p;
>
> -   mru_clear(_git_mru);
> -   for (p = packed_git; p; p = p->next)
> -   mru_append(_git_mru, p);
> +   for (p = packed_git; p; p = p->next) {
> +   list_add_tail(>mru, _git_mru);
> +   }

Unnecessary braces on for-loop.

>  }

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

[Eric Sunshine]

On Fri, Jan 19, 2018 at 9:19 AM, Phillip Wood  wrote:
> Check that cherry-pick and rebase call the 'prepare-commit-msg' hook
> correctly. [...]
>
> Signed-off-by: Phillip Wood 
> ---
> diff --git 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_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 $(seq 1 13)

For portability, use $(test_seq ...) rather than $(seq ...).

> +   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
> +'

Re: git 2.16.0 segfaults on clone of specific repo

[Eric Sunshine]

On Fri, Jan 19, 2018 at 7:23 PM, brian m. carlson
 wrote:
> On Fri, Jan 19, 2018 at 07:15:45PM -0500, Eric Sunshine wrote:
>> Are you planning on submitting the test as a proper patch as follow-up
>> to [1]? I couldn't quite decide in which test script it should reside.
>
> Sure, I can.  Since you wrote it, may I have permission to add your
> sign-off?

You may...

Signed-off-by: Eric Sunshine 

> I'll try to set up a temporary FAT device and verify that I can
> reproduce the problem on Linux as well.

Re: git 2.16.0 segfaults on clone of specific repo

[brian m. carlson]

On Fri, Jan 19, 2018 at 07:15:45PM -0500, Eric Sunshine wrote:
> Oh, I agree. My original question of its practical value was based
> upon my belief that the full test suite is very rarely run on MacOS
> (partly because there are so few Git developers on MacOS and partly
> because it runs so slowly on the platform, though not nearly as slowly
> as on Windows). However, as soon as I hit "Send", it hit me that the
> problem would also manifest on Windows, and we know that Dscho runs
> the tests regularly on Windows, so I changed my mind. I did consider
> Travis but wasn't sure if it was testing on case-insensitive
> filesystems.
> 
> Are you planning on submitting the test as a proper patch as follow-up
> to [1]? I couldn't quite decide in which test script it should reside.

Sure, I can.  Since you wrote it, may I have permission to add your
sign-off?

I'll try to set up a temporary FAT device and verify that I can
reproduce the problem on Linux as well.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: git 2.16.0 segfaults on clone of specific repo

[Eric Sunshine]

On Fri, Jan 19, 2018 at 12:25 PM, Todd Zullinger  wrote:
> Eric Sunshine wrote:
>> Nice detective work. This particular manifestation is caught by the
>> following test which fails without brian's patch on MacOS (and
>> presumably Windows) and succeeds with it. On Linux and BSD, it will of
>> course succeed always, so I'm not sure how much practical value it
>> has.
>
> The CASE_INSENSITIVE_FS prereq could be used to avoid
> running the test on systems where it won't provide much
> value, couldn't it?

As mentioned in [1], my original questioning of the practical value of
the test was tied to the belief that the full test suite is probably
rarely run on MacOS, and it wasn't until I pressed "Send" that it hit
me that it would manifest on Windows as well, and that Dscho does test
on Windows regularly. Therefore, the test could have value.

At any rate, the test potentially could catch some sort of future
regression even on case-sensitive filesystems, therefore, it would be
better not to hide it behind CASE_INSENSITIVE_FS.

[1]: 
https://public-inbox.org/git/CAPig+cQmWqQWQrRQHHn=3hn6UFzJxT=9d5kknjht_dt8scg...@mail.gmail.com/

Re: git 2.16.0 segfaults on clone of specific repo

[Eric Sunshine]

On Fri, Jan 19, 2018 at 5:31 PM, brian m. carlson
 wrote:
> On Fri, Jan 19, 2018 at 02:40:02AM -0500, Eric Sunshine wrote:
>> Nice detective work. This particular manifestation is caught by the
>> following test which fails without brian's patch on MacOS (and
>> presumably Windows) and succeeds with it. On Linux and BSD, it will of
>> course succeed always, so I'm not sure how much practical value it
>> has.
>
> I'd argue that it's a worthwhile test to have, since it will fail on
> those systems where it's going to be a problem.  Furthermore, people do
> run the tests (as does Travis) on case-insensitive file systems during
> the development cycle, so if we break something in the future, someone
> will notice while we're still in the development cycle.

Oh, I agree. My original question of its practical value was based
upon my belief that the full test suite is very rarely run on MacOS
(partly because there are so few Git developers on MacOS and partly
because it runs so slowly on the platform, though not nearly as slowly
as on Windows). However, as soon as I hit "Send", it hit me that the
problem would also manifest on Windows, and we know that Dscho runs
the tests regularly on Windows, so I changed my mind. I did consider
Travis but wasn't sure if it was testing on case-insensitive
filesystems.

Are you planning on submitting the test as a proper patch as follow-up
to [1]? I couldn't quite decide in which test script it should reside.

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

Hello There

[FINANCE CAPITAL]

 UNSECURED BUSINESS/PERSONAL LOAN BY LOAN CAPITAL FINANCE
  - NO COLLATERAL
  - MINIMUM DOCUMENTATION
  - BUSINESS LOAN UP TO FIVE(5) MILLION US DOLLARS

  CONTACT US TODAY VIA EMAIL: financecapital...@mail.com

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

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

[Gargi Sharma]

On Fri, Jan 19, 2018 at 9:46 PM, Jeff King  wrote:
>
> On Mon, Jan 15, 2018 at 08:46:25PM -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.
> >
> > Another discussion, here
> > (https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/)
> > was on what has to be done with the next pointer of packed git type
> > inside the
> > packed_git structure. It can be removed _given_ that no one needs to
> > access the list in order and can be sent as another patch.
>
> Thanks for picking this up again. I agree that this is probably a good
> stopping point for now, as I think just combining this with the 'next'
> pointer may carry more side effects.
Agreed, hence just thought that if the discussion is started again, we
can point them
to the email thread.
>
> Aside from the braces thing that Christian mentioned (and the missing
> signoff), this all looks good to me.
Thanks, made the changes and sent a v3,

Best,
Gargi
>
> -Peff

[PATCH v3] 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 
---
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.

The commit has been built on top of ot/mru-on-list branch.

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|  9 +
 list.h |  7 +++
 mru.c  | 27 ---
 mru.h  | 40 
 packfile.c | 18 +-
 sha1_file.c|  1 -
 8 files changed, 25 insertions(+), 87 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..188ba3e 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_to_front(>mru, _git_mru);
if (want != -1)
return want;
}
diff --git a/cache.h b/cache.h
index 49b083e..1a275ae 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,10 @@ 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/list.h b/list.h
index eb60119..5129b0a 100644
--- a/list.h
+++ b/list.h
@@ -93,6 +93,13 @@ static inline void list_move(struct list_head *elem, struct 
list_head *head)
list_add(elem, head);
 }
 
+/* Move to the front of the list. */
+static inline void list_move_to_front(struct list_head *elem, struct list_head 
*head)
+{
+   list_del(elem);
+   list_add(elem, head);
+}
+
 /* Replace an old entry. */
 static inline void list_replace(struct list_head *old, struct list_head *newp)
 {
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) {
-   

RE: [PATCH v2 4/6] Force test suite traps to be cleared for NonStop ksh

[Randall S. Becker]

On January 19, 2018 5:29 PM, I wrote:
> On January 19, 2018 4:27 PM, Jeff King wrote:
> > On Fri, Jan 19, 2018 at 12:34:04PM -0500, randall.s.bec...@rogers.com
> wrote:
> >
> > > From: "Randall S. Becker" 
> > >
> > > * t/lib-git-daemon.sh: fix incompatibilities with ksh traps not being
> > >   cleared automatically on platform. This caused tests to seem to fail
> > >   while actually succeeding.
> > >
> > > Signed-off-by: Randall S. Becker 
> > > ---
> > >  t/lib-git-daemon.sh | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh index
> > > 987d40680..955beecd9 100644
> > > --- a/t/lib-git-daemon.sh
> > > +++ b/t/lib-git-daemon.sh
> > > @@ -68,6 +68,7 @@ start_git_daemon() {
> > >   test_skip_or_die $GIT_TEST_GIT_DAEMON \
> > >   "git daemon failed to start"
> > >   fi
> > > + trap '' EXIT
> > >  }
> >
> > I don't think this can be right. The way these traps are supposed to work 
> > is:
> >
> >   - as soon as test-lib.sh is loaded, we "trap die EXIT" to catch an
> > accidental death/exit from one of the scripts
> >
> >   - when test_done is called, we clear the trap (i.e., it is OK to exit
> > now because the script has given us a positive indication that all
> > tests have been run)
> >
> >   - while the child git-daemon is running, we'd want to clean up after
> > ourselves. So during start_git_daemon() we add our cleanup (followed
> > by the original "die", because shell traps don't push onto a stack).
> > And then at stop_git_daemon(), we restore the original die trap.
> >
> > But this patch means that while git-daemon is running, we have no trap at
> all!
> > That means that a failed test which causes us to exit would leave a
> > child daemon running.
> >
> > Furthermore, both of these functions now drop the 'die' trap entirely,
> > meaning the script would fail to notice premature exit from one of the
> > test snippets.
> >
> > So while this may be papering over a problem on NonStop, I think it's
> > breaking the intent of the traps.
> >
> > I'm not sure what the root of the problem is, but it sounds like ksh
> > may be triggering EXIT traps when we don't expect (during function
> returns?
> > Subshell exits? Other?)
> 
> The "unexpected" EXIT traps are exactly the issue we found when working
> with the platform support team. There was some discussion about what the
> right expectation was, and I was not able to have a change made to ksh on
> the platform. The problem may have a similar (identical?) root cause to the
> Perl exit issues we are also experiencing that is in their hands. I'm 
> currently
> retesting without this change (results in 36 hours ☹ ). Is there a preferred
> way you would like me to bypass this except on NonStop? I can add a check
> based on uname.

The option that may work, if the tests that are currenting running until Sunday 
(sadly) fail miserably, is to use:

if [ `uname` = "NONSTOP_KERNEL" ]; then trap '' EXIT; fi

or perhaps to add a descriptive function along those lines. We have had two 
major operating system upgrades since the original case relating to ksh traps, 
so perhaps things might improve. Our baseline is that there are currently 6 
breaks (t1308#23, t1405#52, t9001#31/106/107/134), most of which have been 
traced back to perl completion codes.

Cheers,
Randall
P.S. I am happy to explain why the tests perform the at the rate they do on the 
development machines I have, if anyone is interested, although dissertations 
might be involved 

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

[Nit] Lots of enumerated type warnings

[Randall S. Becker]

So here a bit of a nit or nano-quibble that I have. Call it my "warnings
OCD" if you want. I'm seeing an increase in the enumerated type warnings
coming from my use of the c99 compiler for compiling git over time (loads
more for 2.16.0 compared to 2.3.7 when I took it on). What is the general
feeling on these? I would be willing do static casts rather than see the
warnings, mostly because I advocate in public that warnings are actually
future potential errors, so clean compiles are better. I don't see this
conflicting with anything in gcc. Is there a desire/need to clean up this
stuff? I can take a stab at gradually cleaning this up when $DAYJOB and
#FAMILY don't conflict.

Although, given the choice, I'd rather look into that whole --via concept
from a different thread ;-)

Cheers,
Randall

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

Re: [PATCH] repository: pre-initialize hash algo pointer

[brian m. carlson]

On Fri, Jan 19, 2018 at 11:24:24AM -0800, Junio C Hamano wrote:
> Eric Sunshine  writes:
> > Now that we know (due to Duy's excellent detective work[1]) that the
> > trigger is files with names differing only in case on case-insensitive
> > filesystems, the commit message can be updated appropriately.
> 
> Thanks.  Let me apply the following and do a 2.16.1, hopefully by
> the end of day or mid tomorrow at the latest.  Test to protect the
> fix can come as a separate follow-up patch.

This looks good with Eric's suggested changes.

Again, my apologies for the breakage and the inconvenience involved.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

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

[Randall S. Becker]

> -Original Message-
> From: randall.s.bec...@rogers.com [mailto:randall.s.bec...@rogers.com]
> Sent: January 19, 2018 6:00 PM
> To: git@vger.kernel.org
> Cc: Randall S. Becker 
> Subject: [PATCH v3 0/6] Force pipes to flush immediately on NonStop
> platform
> 
> From: "Randall S. Becker" 
> 
> * wrapper.c: called setbuf(stream,NULL) to force pipe flushes not
>   enabled by default on the NonStop platform. This applies only
>   to the NonStop platform guarded by #ifdef __TANDEM.
> 
> Signed-off-by: Randall S. Becker 
> ---
>  wrapper.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/wrapper.c b/wrapper.c
> index d20356a77..671cbb4b4 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
>   FILE *stream = fdopen(fd, mode);
>   if (stream == NULL)
>   die_errno("Out of memory? fdopen failed");
> +#ifdef __TANDEM
> + setbuf(stream,NULL);
> +#endif
>   return stream;
>  }
> 
> --
> 2.16.0.31.gf1a482c

This is a replacement for the v2 patch based on Stefan's suggestions.
Cheers,
Randall

[PATCH v3 0/6] Force pipes to flush immediately on NonStop platform

[randall . s . becker]

From: "Randall S. Becker" 

* wrapper.c: called setbuf(stream,NULL) to force pipe flushes not
  enabled by default on the NonStop platform. This applies only
  to the NonStop platform guarded by #ifdef __TANDEM.

Signed-off-by: Randall S. Becker 
---
 wrapper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/wrapper.c b/wrapper.c
index d20356a77..671cbb4b4 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
FILE *stream = fdopen(fd, mode);
if (stream == NULL)
die_errno("Out of memory? fdopen failed");
+#ifdef __TANDEM
+   setbuf(stream,NULL);
+#endif
return stream;
 }
 
-- 
2.16.0.31.gf1a482c

RE: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Randall S. Becker]

On January 19, 2018 5:43 PM, Ramsay Jones wrote:
> On 19/01/18 21:20, Jeff King wrote:
> > On Fri, Jan 19, 2018 at 08:28:48PM +, Ramsay Jones wrote:
> >
> >>> diff --git a/remote.c b/remote.c
> >>> index 4e93753e1..c18f9de7f 100644
> >>> --- a/remote.c
> >>> +++ b/remote.c
> >>> @@ -11,6 +11,10 @@
> >>>  #include "mergesort.h"
> >>>  #include "argv-array.h"
> >>>
> >>> +#if defined (__TANDEM)
> >>> +#define __attribute(a)
> >>> +#endif
> >>> +
> >>
> >> Hmm, the only use of __attribute() I can find is in compat/regex/.
> >> In particular, there is no use of __attribute() in regex.c.
> >> [__attribute__() is used in regex.c]
> >>
> >> Is this an old patch which is no longer required?
> >>
> >> puzzled.
> 
> heh, I only just noticed that I (twice) wrote regex.c when I meant remote.c
> instead. Hopefully, that didn't cause too much confusion!
> 
> > I'm puzzled, too. The actual gcc thing is __attribute__(), and we
> > already turn that into a noop via macro expansion if __GNUC__ is not
> > defined (in git-compat-util.h, but see below).
> >
> > __attribute(), without the trailing underscores, is used internally by
> > the regex compat code (but it also converts that into a noop on
> > non-GNUC platforms)>
> 
> Indeed.
> 
> > However the logic in git-compat-util is weird:
> >
> >   #if defined(__HP_cc) && (__HP_cc >= 61000)
> >   #define NORETURN __attribute__((noreturn))
> >   #define NORETURN_PTR
> >   #elif defined(__GNUC__) && !defined(NO_NORETURN)
> >   #define NORETURN __attribute__((__noreturn__))
> >   #define NORETURN_PTR __attribute__((__noreturn__))
> >   #elif defined(_MSC_VER)
> >   #define NORETURN __declspec(noreturn)
> >   #define NORETURN_PTR
> >   #else
> >   #define NORETURN
> >   #define NORETURN_PTR
> >   #ifndef __GNUC__
> >   #ifndef __attribute__
> >   #define __attribute__(x)
> >   #endif
> >   #endif
> >   #endif
> >
> > Most of the conditional is dealing with NORETURN, but then we stick
> > the __attribute()__ handling in the "else" block. Is it possible that
> > this platform triggers __HP_cc, but doesn't actually understand
> > __attribute__?
> 
> That seems unlikely. However, that conditional looks a mess ... ;-)

Very messy and confusing and it is working properly now, so... consider this 
patch gone ;-)

Cheers,
Randall

Hello There

[FINANCE CAPITAL]

 UNSECURED BUSINESS/PERSONAL LOAN BY LOAN CAPITAL FINANCE
  - NO COLLATERAL
  - MINIMUM DOCUMENTATION
  - BUSINESS LOAN UP TO FIVE(5) MILLION US DOLLARS

  CONTACT US TODAY VIA EMAIL: financecapital...@mail.com

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

Re: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Ramsay Jones]

On 19/01/18 21:20, Jeff King wrote:
> On Fri, Jan 19, 2018 at 08:28:48PM +, Ramsay Jones wrote:
> 
>>> diff --git a/remote.c b/remote.c
>>> index 4e93753e1..c18f9de7f 100644
>>> --- a/remote.c
>>> +++ b/remote.c
>>> @@ -11,6 +11,10 @@
>>>  #include "mergesort.h"
>>>  #include "argv-array.h"
>>>  
>>> +#if defined (__TANDEM)
>>> +#define __attribute(a)
>>> +#endif
>>> +
>>
>> Hmm, the only use of __attribute() I can find is in compat/regex/.
>> In particular, there is no use of __attribute() in regex.c.
>> [__attribute__() is used in regex.c]
>>
>> Is this an old patch which is no longer required?
>>
>> puzzled.

heh, I only just noticed that I (twice) wrote regex.c when I meant
remote.c instead. Hopefully, that didn't cause too much confusion!

> I'm puzzled, too. The actual gcc thing is __attribute__(), and we
> already turn that into a noop via macro expansion if __GNUC__ is not
> defined (in git-compat-util.h, but see below).
> 
> __attribute(), without the trailing underscores, is used internally by
> the regex compat code (but it also converts that into a noop on non-GNUC
> platforms)>

Indeed.

> However the logic in git-compat-util is weird:
> 
>   #if defined(__HP_cc) && (__HP_cc >= 61000)
>   #define NORETURN __attribute__((noreturn))
>   #define NORETURN_PTR
>   #elif defined(__GNUC__) && !defined(NO_NORETURN)
>   #define NORETURN __attribute__((__noreturn__))
>   #define NORETURN_PTR __attribute__((__noreturn__))
>   #elif defined(_MSC_VER)
>   #define NORETURN __declspec(noreturn)
>   #define NORETURN_PTR
>   #else
>   #define NORETURN
>   #define NORETURN_PTR
>   #ifndef __GNUC__
>   #ifndef __attribute__
>   #define __attribute__(x)
>   #endif
>   #endif
>   #endif
> 
> Most of the conditional is dealing with NORETURN, but then we stick the
> __attribute()__ handling in the "else" block. Is it possible that this
> platform triggers __HP_cc, but doesn't actually understand
> __attribute__?

That seems unlikely. However, that conditional looks a mess ... ;-)

ATB,
Ramsay Jones

RE: [PATCH v2 1/6] DO NOT APPLY Bypass GCC attributes on NonStop platform where used.

[Randall S. Becker]

On January 19, 2018 3:29 PM, Ramsay Jones wrote:
> On 19/01/18 17:34, randall.s.bec...@rogers.com wrote:
> > From: "Randall S. Becker" 
> >
> > * remote.c: force ignoring of GCC __attribute construct not supported
> >   by c99 by defining it as an empty CPP macro.
> >
> > Signed-off-by: Randall S. Becker 
> > ---
> >  remote.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/remote.c b/remote.c
> > index 4e93753e1..c18f9de7f 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -11,6 +11,10 @@
> >  #include "mergesort.h"
> >  #include "argv-array.h"
> >
> > +#if defined (__TANDEM)
> > +#define __attribute(a)
> > +#endif
> > +
> 
> Hmm, the only use of __attribute() I can find is in compat/regex/.
> In particular, there is no use of __attribute() in regex.c.
> [__attribute__() is used in regex.c]
> 
> Is this an old patch which is no longer required?
> 
> puzzled.

After investigation, this patch can be dropped. The __attribute__ fix from 
git-compat-utils.h is actually being picked up correctly now (unlike our 2.8.5 
port when it was required). I am suspecting that this was blocked by a 
configuration setting in config.mak.uname that got fixed when I got my hands on 
that file for a cleanup. The path through there is via #ifndef __GNUC__.

Cheers,
Randall

Re: git 2.16.0 segfaults on clone of specific repo

[brian m. carlson]

On Fri, Jan 19, 2018 at 02:40:02AM -0500, Eric Sunshine wrote:
> Nice detective work. This particular manifestation is caught by the
> following test which fails without brian's patch on MacOS (and
> presumably Windows) and succeeds with it. On Linux and BSD, it will of
> course succeed always, so I'm not sure how much practical value it
> has.
> 
> --- >8 ---
> hex2oct() {
>   perl -ne 'printf "\\%03o", hex for /../g'
> }
> 
> test_expect_success 'clone on case-insensitive fs' '
>   o=$(git hash-object -w --stdint=$(printf "100644 X\0${o}100644 x\0${o}" |
>  git hash-object -w -t tree --stdin) &&
>   c=$(git commit-tree -m bogus $t) &&
>   git update-ref refs/heads/bogus $c &&
>   git clone -b bogus . bogus
> '
> --- >8 ---

I'd argue that it's a worthwhile test to have, since it will fail on
those systems where it's going to be a problem.  Furthermore, people do
run the tests (as does Travis) on case-insensitive file systems during
the development cycle, so if we break something in the future, someone
will notice while we're still in the development cycle.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

RE: [PATCH v2 4/6] Force test suite traps to be cleared for NonStop ksh

[Randall S. Becker]

On January 19, 2018 4:27 PM, Jeff King wrote:
> On Fri, Jan 19, 2018 at 12:34:04PM -0500, randall.s.bec...@rogers.com wrote:
> 
> > From: "Randall S. Becker" 
> >
> > * t/lib-git-daemon.sh: fix incompatibilities with ksh traps not being
> >   cleared automatically on platform. This caused tests to seem to fail
> >   while actually succeeding.
> >
> > Signed-off-by: Randall S. Becker 
> > ---
> >  t/lib-git-daemon.sh | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh index
> > 987d40680..955beecd9 100644
> > --- a/t/lib-git-daemon.sh
> > +++ b/t/lib-git-daemon.sh
> > @@ -68,6 +68,7 @@ start_git_daemon() {
> > test_skip_or_die $GIT_TEST_GIT_DAEMON \
> > "git daemon failed to start"
> > fi
> > +   trap '' EXIT
> >  }
> 
> I don't think this can be right. The way these traps are supposed to work is:
> 
>   - as soon as test-lib.sh is loaded, we "trap die EXIT" to catch an
> accidental death/exit from one of the scripts
> 
>   - when test_done is called, we clear the trap (i.e., it is OK to exit
> now because the script has given us a positive indication that all
> tests have been run)
> 
>   - while the child git-daemon is running, we'd want to clean up after
> ourselves. So during start_git_daemon() we add our cleanup (followed
> by the original "die", because shell traps don't push onto a stack).
> And then at stop_git_daemon(), we restore the original die trap.
> 
> But this patch means that while git-daemon is running, we have no trap at all!
> That means that a failed test which causes us to exit would leave a child
> daemon running.
> 
> Furthermore, both of these functions now drop the 'die' trap entirely,
> meaning the script would fail to notice premature exit from one of the test
> snippets.
> 
> So while this may be papering over a problem on NonStop, I think it's
> breaking the intent of the traps.
> 
> I'm not sure what the root of the problem is, but it sounds like ksh may be
> triggering EXIT traps when we don't expect (during function returns?
> Subshell exits? Other?)

The "unexpected" EXIT traps are exactly the issue we found when working with 
the platform support team. There was some discussion about what the right 
expectation was, and I was not able to have a change made to ksh on the 
platform. The problem may have a similar (identical?) root cause to the Perl 
exit issues we are also experiencing that is in their hands. I'm currently 
retesting without this change (results in 36 hours ☹ ). Is there a preferred 
way you would like me to bypass this except on NonStop? I can add a check based 
on uname.

Cheers,
Randall

Re: [PATCH] repository: pre-initialize hash algo pointer

[Junio C Hamano]

Eric Sunshine  writes:

> On Fri, Jan 19, 2018 at 2:24 PM, Junio C Hamano  wrote:
>> Eric Sunshine  writes:
>>> Now that we know (due to Duy's excellent detective work[1]) that the
>>> trigger is files with names differing only in case on case-insensitive
>>> filesystems, the commit message can be updated appropriately.
>>
>> Thanks.  Let me apply the following and do a 2.16.1, hopefully by
>> the end of day or mid tomorrow at the latest.  Test to protect the
>> fix can come as a separate follow-up patch.
>>
>> -- >8 --
>> Subject: [PATCH] repository: pre-initialize hash algo pointer
>> [...]
>> A "git clone" of a project that has two paths that differ only in
>> case suffers from this if it is run on a case insensitive platform.
>> When the command attempts to check out one of these two paths after
>> checking out the other one, the checkout codepath needs to see if
>> the version that is already on the filesystem (which should not
>> happen if the FS were case sensitive), and it needs to exercise the
>> hashing code.
>
> Thanks, the amended commit message makes the reason for the patch more
> concrete. There does seem to be a bit of a grammatical issue, however,
> which makes it difficult to parse. Namely, "already on the filesystem
> (...)" probably was meant to say "already on the filesystem (...) is
> {something}".

Indeed it is incomplete sentence.  The codepath wants to check if
the one on the filesystem is the same as the one that is being
checked out and exercises the hashing code at that point.

Thansk.

Re: [PATCH] files_initial_transaction_commit(): only unlock if locked

[Junio C Hamano]

Jeff King  writes:

> On Thu, Jan 18, 2018 at 02:38:41PM +0100, Mathias Rav wrote:
>
>> Running git clone --single-branch --mirror -b TAGNAME previously
>> triggered the following error message:
>> 
>>  fatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.
>> 
>> This error condition is handled in files_initial_transaction_commit().
>> 
>> 42c7f7ff9 ("commit_packed_refs(): remove call to `packed_refs_unlock()`", 
>> 2017-06-23)
>> introduced incorrect unlocking in the error path of this function,
>> which changes the error message to
>> 
>>  fatal: BUG: packed_refs_unlock() called when not locked
>> 
>> Move the call to packed_refs_unlock() above the "cleanup:" label
>> since the unlocking should only be done in the last error path.
>
> Thanks, this solution looks correct to me. It's pretty low-impact since
> the locking is the second-to-last thing in the function, so we don't
> have to re-add the unlock to a bunch of error code paths. But one
> alternative would be to just do:
>
>   if (packed_refs_is_locked(refs))
>   packed_refs_unlock(refs->packed_ref_store);
>
> in the cleanup section.

Yeah, that may be a more future-proof alternative, and just as you
said the patch as posted would be sufficient, too.

Thanks.

Re: [PATCH v3 1/3] read-cache: fix reading the shared index for other repos

[Junio C Hamano]

Thomas Gummerer  writes:

> read_cache_from() defaults to using the gitdir of the_repository.  As it
> is mostly a convenience macro, having to pass get_git_dir() for every
> call seems overkill, and if necessary users can have more control by
> using read_index_from().

This was a bit painful change, given that some changes in flight do
add new callsites to read_index_from() and they got the function
changed under their feet.

Please double check if I made the right git-dir to be passed to them
when I push out 'pu' in a few hours.

Thanks.

Re: [PATCH v2] diff: add --compact-summary option to complement --stat

[Jeff King]

On Fri, Jan 19, 2018 at 01:20:41PM -0800, Junio C Hamano wrote:

> >  Documentation/merge-config.txt |  4 ++
> >  builtin/merge.c|  2 +
> >  A+x t/t5573-pull-verify-signatures.sh  | 81 
> >  t/t7612-merge-verify-signatures.sh | 45 +
> >  4 files changed, 132 insertions(+)
> 
> I like the concept but given that additions and mode changes are
> rare events, I am not so sure if it is worth always wasting three
> columns like the above.  Assuming that this is solely meant for
> human consumption and machine parsability is of no concern, I
> actually prefer the output format you said you've been using your
> personal fork, e.g.
> 
>  Documentation/merge-config.txt  |  4 ++
>  builtin/merge.c |  2 +
>  t/t5573-pull-verify-signatures.sh (new, +x) | 81 
>  t/t7612-merge-verify-signatures.sh  | 45 +
> 
> That is
> 
>  (1) do not change the starting column at the leftmost end, and
>  (2) do not permanently allocate the columns for "compact" summary.

I think the patch already does (2). In fact, it computes the max
compact-status size (so if you only have "A" and not "A+x", it wastes
only the one column).

I agree that (1) would save space in some cases, though IMHO it's a
little hard to notice.

-Peff

Re: [PATCH v2] diff: add --compact-summary option to complement --stat

[Jeff King]

On Fri, Jan 19, 2018 at 07:26:28AM +0700, Duy Nguyen wrote:

> > (I know this is a bikeshed, so I'm perfectly willing to take "yuck, I
> > don't like that as well" as a response).
> 
> The position of A+x column is exactly where gerrit put it. Though web
> pages have more flexibility than our terminal console so its position
> does not have to be the same. I'm just throwing options out there
> 
> For many years I have this instead
> 
>  t/t5573-pull-verify-signatures.sh (new +x) | 81 
> 
> Another option is just wrap the code in [] to make it look like check
> boxes. But that wastes two more columns
> 
>builtin/merge.c|  2 +
>  [A+x] t/t5573-pull-verify-signatures.sh  | 81 
>t/t7612-merge-verify-signatures.sh | 45 +

Yeah, I almost suggested brackets, but wasn't sure if people would balk
at the extra 2 columns. But they do help it stand out more. Colors would
help, too, as you noted. Though they would not transfer over email, and
I wonder if people would want to use this for format-patch.

> Back to your suggestion, I kinda like the closeness between the +/-
> count and "|" though. The output on 10c78a162f is like this, which
> makes "A" looks a bit umm.. disconnected from the path name?
> 
>   Documentation/RelNotes/2.14.0.txt | A  97 +++
>   GIT-VERSION-GEN   | 2 +-
>   RelNotes  | 2 +-

Yeah, I was trying to get it away from the pathname, since it doesn't
stand out as much. I guess it depends how you think of the "A". To me it
is sensible as a modifier for the line-count change. I.e., My read on
the output above is "here is a path; it was added with 97 lines".

> > One thing I noticed is that --compact-summary by itself does nothing.
> > Should it imply --stat?
> 
> It might go with --numstat or --dirstat in future too. Didn't really
> think hard about this yet. But I probably will go with Eric suggestion
> and keep this in --stat= unless it really makes sense to have
> something like this in --numstat or --dirstat.

I'd think that most consumers of --numstat are not human, and would
just use "--numstat --raw" to get all the information. But I also have
not thought hard about it.

Anyway, thanks for listening. :)

-Peff

Re: [PATCH] repository: pre-initialize hash algo pointer

[Eric Sunshine]

On Fri, Jan 19, 2018 at 2:24 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> Now that we know (due to Duy's excellent detective work[1]) that the
>> trigger is files with names differing only in case on case-insensitive
>> filesystems, the commit message can be updated appropriately.
>
> Thanks.  Let me apply the following and do a 2.16.1, hopefully by
> the end of day or mid tomorrow at the latest.  Test to protect the
> fix can come as a separate follow-up patch.
>
> -- >8 --
> Subject: [PATCH] repository: pre-initialize hash algo pointer
> [...]
> A "git clone" of a project that has two paths that differ only in
> case suffers from this if it is run on a case insensitive platform.
> When the command attempts to check out one of these two paths after
> checking out the other one, the checkout codepath needs to see if
> the version that is already on the filesystem (which should not
> happen if the FS were case sensitive), and it needs to exercise the
> hashing code.

Thanks, the amended commit message makes the reason for the patch more
concrete. There does seem to be a bit of a grammatical issue, however,
which makes it difficult to parse. Namely, "already on the filesystem
(...)" probably was meant to say "already on the filesystem (...) is
{something}".

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

[Jeff King]

On Mon, Jan 15, 2018 at 08:46:25PM -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.
> 
> Another discussion, here
> (https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/)
> was on what has to be done with the next pointer of packed git type
> inside the
> packed_git structure. It can be removed _given_ that no one needs to
> access the list in order and can be sent as another patch.

Thanks for picking this up again. I agree that this is probably a good
stopping point for now, as I think just combining this with the 'next'
pointer may carry more side effects.

Aside from the braces thing that Christian mentioned (and the missing
signoff), this all looks good to me.

-Peff

Re: [PATCH v6 0/7] Trace env variables in run_command()

[Jeff King]

On Thu, Jan 18, 2018 at 04:45:05PM +0700, Nguyễn Thái Ngọc Duy wrote:

> v6 squashes Junio's changes in 6/7 and moves trace_run_command() from
> trace.c to run-command.c.

Thanks, this version looks good to me.

-Peff

Re: [PATCH v2 4/6] Force test suite traps to be cleared for NonStop ksh

[Jeff King]

On Fri, Jan 19, 2018 at 12:34:04PM -0500, randall.s.bec...@rogers.com wrote:

> From: "Randall S. Becker" 
> 
> * t/lib-git-daemon.sh: fix incompatibilities with ksh traps not being
>   cleared automatically on platform. This caused tests to seem to fail
>   while actually succeeding.
> 
> Signed-off-by: Randall S. Becker 
> ---
>  t/lib-git-daemon.sh | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
> index 987d40680..955beecd9 100644
> --- a/t/lib-git-daemon.sh
> +++ b/t/lib-git-daemon.sh
> @@ -68,6 +68,7 @@ start_git_daemon() {
>   test_skip_or_die $GIT_TEST_GIT_DAEMON \
>   "git daemon failed to start"
>   fi
> + trap '' EXIT
>  }

I don't think this can be right. The way these traps are supposed to
work is:

  - as soon as test-lib.sh is loaded, we "trap die EXIT" to catch an
accidental death/exit from one of the scripts

  - when test_done is called, we clear the trap (i.e., it is OK to exit
now because the script has given us a positive indication that all
tests have been run)

  - while the child git-daemon is running, we'd want to clean up after
ourselves. So during start_git_daemon() we add our cleanup (followed
by the original "die", because shell traps don't push onto a stack).
And then at stop_git_daemon(), we restore the original die trap.

But this patch means that while git-daemon is running, we have no trap
at all! That means that a failed test which causes us to exit would
leave a child daemon running.

Furthermore, both of these functions now drop the 'die' trap entirely,
meaning the script would fail to notice premature exit from one of the
test snippets.

So while this may be papering over a problem on NonStop, I think it's
breaking the intent of the traps.

I'm not sure what the root of the problem is, but it sounds like ksh may
be triggering EXIT traps when we don't expect (during function returns?
Subshell exits? Other?)

-Peff

Re: [PATCH v2] diff: add --compact-summary option to complement --stat

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

>  Documentation/merge-config.txt |  4 +
>  builtin/merge.c|  2 +
>A t/t5573-pull-verify-signatures.sh  | 81 ++
>  t/t7612-merge-verify-signatures.sh | 45 ++
>4 files changed, 132 insertions(+)
> ...
> With this tweak, the actual printout is like this
>
>  Documentation/merge-config.txt |  4 ++
>  builtin/merge.c|  2 +
>  A+x t/t5573-pull-verify-signatures.sh  | 81 
>  t/t7612-merge-verify-signatures.sh | 45 +
>  4 files changed, 132 insertions(+)

I like the concept but given that additions and mode changes are
rare events, I am not so sure if it is worth always wasting three
columns like the above.  Assuming that this is solely meant for
human consumption and machine parsability is of no concern, I
actually prefer the output format you said you've been using your
personal fork, e.g.

 Documentation/merge-config.txt  |  4 ++
 builtin/merge.c |  2 +
 t/t5573-pull-verify-signatures.sh (new, +x) | 81 
 t/t7612-merge-verify-signatures.sh  | 45 +

That is

 (1) do not change the starting column at the leftmost end, and
 (2) do not permanently allocate the columns for "compact" summary.

Instead, the above may be (a) just stealing the columns needed for
"(new, +x)" from the pathname portion of the output, and/or (2)
widening the pathname portion of the output for the whole thing
while doing so.

Re: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Jeff King]

On Fri, Jan 19, 2018 at 08:28:48PM +, Ramsay Jones wrote:

> > diff --git a/remote.c b/remote.c
> > index 4e93753e1..c18f9de7f 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -11,6 +11,10 @@
> >  #include "mergesort.h"
> >  #include "argv-array.h"
> >  
> > +#if defined (__TANDEM)
> > +#define __attribute(a)
> > +#endif
> > +
> 
> Hmm, the only use of __attribute() I can find is in compat/regex/.
> In particular, there is no use of __attribute() in regex.c.
> [__attribute__() is used in regex.c]
> 
> Is this an old patch which is no longer required?
> 
> puzzled.

I'm puzzled, too. The actual gcc thing is __attribute__(), and we
already turn that into a noop via macro expansion if __GNUC__ is not
defined (in git-compat-util.h, but see below).

__attribute(), without the trailing underscores, is used internally by
the regex compat code (but it also converts that into a noop on non-GNUC
platforms)>

However the logic in git-compat-util is weird:

  #if defined(__HP_cc) && (__HP_cc >= 61000)
  #define NORETURN __attribute__((noreturn))
  #define NORETURN_PTR
  #elif defined(__GNUC__) && !defined(NO_NORETURN)
  #define NORETURN __attribute__((__noreturn__))
  #define NORETURN_PTR __attribute__((__noreturn__))
  #elif defined(_MSC_VER)
  #define NORETURN __declspec(noreturn)
  #define NORETURN_PTR
  #else
  #define NORETURN
  #define NORETURN_PTR
  #ifndef __GNUC__
  #ifndef __attribute__
  #define __attribute__(x)
  #endif
  #endif
  #endif

Most of the conditional is dealing with NORETURN, but then we stick the
__attribute()__ handling in the "else" block. Is it possible that this
platform triggers __HP_cc, but doesn't actually understand
__attribute__?

-Peff

Re: [PATCH v3 0/3] fixes for split index mode

[Thomas Gummerer]

On 01/19, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > Friendly ping on this series now that 2.16 is out :) Is there anything
> > in this series (up to 3/3, 4/3 can be dropped now that Duy fixed it in
> > a nicer way) that still needs updating?  It fixes a few bugs in split
> > index mode with submodules/worktrees, so it would be nice to get this
> > reviewed/merged.
> 
> I was wondering about the same thing.  Especially it wasn't very
> clear to me what Duy's replacement was meant to replace and how well
> it was supposed to work with the rest of your series.

Sorry about the confusion.  4/3 was replaced by Duy's series, but the
rest of this series is independent of those patches.

> Let's drop 4/3 and queue 1-3/3 for now.

Thanks!

Re: [PATCH] dir.c: print correct errno when opendir() fails

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> The call invalidate_directory() between opendir() and warning_errno() in
> theory could make some system calls and change errno. Prevent that by
> warning immediately after opendir().
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  This is on top of nd/fix-untracked-cache-invalidation which is now on
>  'next'. Sorry I waited too long to send the replacement and it's now
>  too late.

Well, we'll see a rewind of 'next' soonish anyway, so you can just
tell me to tentatively kick it back to 'pu' to be replaced with a
reroll if you prefer.




>  dir.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index dd1e50c328..55736d3e2a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1795,14 +1795,14 @@ static int open_cached_dir(struct cached_dir *cdir,
>   return 0;
>   c_path = path->len ? path->buf : ".";
>   cdir->fdir = opendir(c_path);
> + if (!cdir->fdir)
> + warning_errno(_("could not open directory '%s'"), c_path);
>   if (dir->untracked) {
>   invalidate_directory(dir->untracked, untracked);
>   dir->untracked->dir_opened++;
>   }
> - if (!cdir->fdir) {
> - warning_errno(_("could not open directory '%s'"), c_path);
> + if (!cdir->fdir)
>   return -1;
> - }
>   return 0;
>  }

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

[Junio C Hamano]

Christian Couder  writes:

> On Tue, Jan 16, 2018 at 2:46 AM, 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.
>
> You might want to say that this provides a significant code reduction
> which shows that the mru API is not a very useful abstraction anymore.
>
>> Another discussion, here
>> (https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/)
>> was on what has to be done with the next pointer of packed git type
>
> I think using "pointer to a 'struct packed_git'" instead of "pointer
> of packed git type" would be clearer here, but anyway see below.
>
>> inside the
>> packed_git structure. It can be removed _given_ that no one needs to
>> access the list in order and can be sent as another patch.
>
> I don't think it's worth pointing to a discussion about a future
> improvement in the commit message. You could perhaps even remove all
> the above paragraph as this commit is valuable and self contained
> enough by itself.

True. 

If it is summarizing conclusion of the earlier discussion, that is a
different matter, though.

It is perfectly OK to have it under "---" line, even if it is merely
voicing author's opinion, by the way.  It can serve as a good
discussion (re-)starter.


>> ---

Missing sign-off?

>> Changes in v2:
>> - Add a move to front function to the list API.
>> - Remove memory leak.
>> - Remove redundant remove operations on the list.
>>
>> The commit has been built on top of ot/mru-on-list branch.
>
> Nice!
>
>>  Makefile   |  1 -
>>  builtin/pack-objects.c | 12 ++--
>>  cache.h|  9 +
>>  list.h |  7 +++
>>  mru.c  | 27 ---
>>  mru.h  | 40 
>>  packfile.c | 18 +-
>>  sha1_file.c|  1 -
>>  8 files changed, 27 insertions(+), 88 deletions(-)
>>  delete mode 100644 mru.c
>>  delete mode 100644 mru.h
>
> Very nice!

Yes, nice reduction.

>
> [...]
>
>> @@ -1030,8 +1029,9 @@ static int want_object_in_pack(const unsigned char 
>> *sha1,
>> *found_pack = p;
>> }
>> want = want_found_object(exclude, p);
>> -   if (!exclude && want > 0)
>> -   mru_mark(_git_mru, entry);
>> +   if (!exclude && want > 0) {
>> +   list_move_to_front(>mru, _git_mru);
>> +   }
>
> Style: we usually remove brackets when there is one line after the
> if(...) line. (See the 2 lines that you delete.)
>
> Otherwise the patch looks good to me.
>
> Thanks,
> Christian.

Re: [PATCH 9/8] [DO NOT APPLY, but squash?] git-rebase--interactive: clarify arguments

[Junio C Hamano]

Johannes Schindelin  writes:

> Good idea! I would rather do it as an introductory patch (that only
> converts the existing list).
>
> As to `merge`: it is a bit more complicated ;-)
>
>   m, merge  (  | "..." ) []
>   create a merge commit using the original merge commit's
>   message (or the oneline, if "-" is given). Use a quoted
>   list of commits to be merged for octopus merges.

Is it just the message that is being reused?  

Aren't the trees of the original commit and its parents participate
in creating the tree of the recreated merge?  One way to preserve an
originally evil merge is to notice how it was made by taking the
difference between the result of mechanical merge of original merge
parents and the original merge result, and carry it forward when
recreating the merge across new parents.  Just being curious.

Re: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Ramsay Jones]

On 19/01/18 17:34, randall.s.bec...@rogers.com wrote:
> From: "Randall S. Becker" 
> 
> * remote.c: force ignoring of GCC __attribute construct not supported
>   by c99 by defining it as an empty CPP macro.
> 
> Signed-off-by: Randall S. Becker 
> ---
>  remote.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/remote.c b/remote.c
> index 4e93753e1..c18f9de7f 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -11,6 +11,10 @@
>  #include "mergesort.h"
>  #include "argv-array.h"
>  
> +#if defined (__TANDEM)
> +#define __attribute(a)
> +#endif
> +

Hmm, the only use of __attribute() I can find is in compat/regex/.
In particular, there is no use of __attribute() in regex.c.
[__attribute__() is used in regex.c]

Is this an old patch which is no longer required?

puzzled.

ATB,
Ramsay Jones

>  enum map_direction { FROM_SRC, FROM_DST };
>  
>  static struct refspec s_tag_refspec = {
> 

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

[Junio C Hamano]

Johannes Schindelin  writes:

> Think of --recreate-merges as "--preserve-merges done right". It
> introduces new verbs for the todo list, `label`, `reset` and `merge`.
> For a commit topology like this:
>
> A - B - C
>   \   /
> D
>
> the generated todo list would look like this:
>
> # branch D
> pick 0123 A
> label branch-point
> pick 1234 D
> label D
>
> reset branch-point
> pick 2345 B
> merge 3456 D C

Yup.  I've seen this design talked about on list in the past, and
I've always felt that this is "sequencer done right".

At the first glance, it may feel somewhat unsatisfying that "merge"
has to say effects of which commits should be reflected in the
result and which commot to take the log message from, i.e.
(recreated)D is merged to form the resulting tree, and 3456=C is
used for the log, to recreate C in the above example, while "pick"
always uses the same commit for both, i.e. recreated B inherits both
the changes and log message from the original B=2345 (or depending
on the readers' point of view, "merge" is allowed to use two
different commits, while "pick" is always limited to the same one).

But I think this distinction is probably fundamental and I am not
opposed to it at all.  The result of "pick" has only one parent, and
the parent is determined only by the previous actions and not by
anything on the "pick" line in the todo list.  But the result of
"merge" has to record all the other parents, and only the first
parent is determined implicitly by the previous actions.  We need to
tell the "merge" command about "3456=C" in order to recreate the
effect of original merge commit (i.e. changes between B and C) as
well as its log message, and we also need to tell it about label "D"
that it is the "other parent" that need to be recorded.

Obviously "merge" command syntax should allow recreating an octopus,
so whenever I said "two" in the above, I meant "N".  The original
merge commit is needed so that the effect to replay (roughly: a
patch going to the original merge result from its first parent) can
be learned from the existing history, and all the other "N-1"
parents needs to be given (and they must have been already created
in the todo list) so that the resulting recreated merge can be
recorded with them as parents (in addition to the first parent that
is implicitly given as the result of all the previous steps).

One interesting (and probably useful) thing to notice is that if A
were not rebased in the above sample picture, and only B were the
one that was tweaked, then a recreated C may use the same original D
as its side parent, and the mechanism outlined above naturally can
support it by allowing an un-rewritten commit to be given as a side
parent when "merge" is redoing C.

I probably won't have time to actually look at the code for a few
days, but I am reasonably excited about the topic ;-)

Thanks.

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Jeff King]

On Fri, Jan 19, 2018 at 10:47:57AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I also think %(deltabase) does make sense for anything that points to an
> > object. I suspect it's not all that _useful_ for for-each-ref, but that
> > doesn't mean we can't return the sensible thing if somebody asks for it.
> 
> This may not be a new issue (or any issue at all), but is the
> ability to learn deltabase make any sense in the first place?
> 
> What should the code do when an object has three copies in the
> repo, i.e. one as a base object (or a loose one), another as a
> delta against an object, and the third one as a delta against
> a different object?

This was a known issue when I introduced %(deltabase). The documentation
explicitly calls this out and makes no promises about which copy we
describe.

The %(objectsize:disk) atom has the same issue, too. See the CAVEATS
section of git-cat-file(1).

-Peff

Re: [PATCH v2 4/6] Force test suite traps to be cleared for NonStop ksh

[Stefan Beller]

On Fri, Jan 19, 2018 at 9:34 AM,   wrote:
> From: "Randall S. Becker" 
>
> * t/lib-git-daemon.sh: fix incompatibilities with ksh traps not being
>   cleared automatically on platform.

Which platform? Do we need to guard it for other platforms?
(I guess we don't we have traps in some other places and it is
POSIX)

>  This caused tests to seem to fail
>   while actually succeeding.
>
> Signed-off-by: Randall S. Becker 
> ---
>  t/lib-git-daemon.sh | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
> index 987d40680..955beecd9 100644
> --- a/t/lib-git-daemon.sh
> +++ b/t/lib-git-daemon.sh
> @@ -68,6 +68,7 @@ start_git_daemon() {
> test_skip_or_die $GIT_TEST_GIT_DAEMON \
> "git daemon failed to start"
> fi
> +   trap '' EXIT
>  }
>
>  stop_git_daemon() {
> @@ -89,4 +90,6 @@ stop_git_daemon() {
> fi
> GIT_DAEMON_PID=
> rm -f git_daemon_output
> +
> +   trap '' EXIT
>  }
> --
> 2.16.0.31.gf1a482c
>

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

[Stefan Beller]

On Fri, Jan 19, 2018 at 9:33 AM,   wrote:
> From: "Randall S. Becker" 
>
> * wrapper.c: called setbuf(stream,0) to force pipe flushes not enabled by
>   default on the NonStop platform.
>

Due to my review of a previous patch, I now know about the __TANDEM
directive and why we use this to guard this line. :)

The setbuf(0) is easier than sprinkling (guarded) flushes all over the code,
so that seems like a clean solution, which we currently don't use.
(it occurred twice in history, see eac14f8909 (Win32: Thread-safe
windows console output, 2012-01-14) for its introduction and fcd428f4a9
(Win32: fix broken pipe detection, 2012-03-01) for its deletion).

> Signed-off-by: Randall S. Becker 
> ---
>  wrapper.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/wrapper.c b/wrapper.c
> index d20356a77..671cbb4b4 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
> FILE *stream = fdopen(fd, mode);
> if (stream == NULL)
> die_errno("Out of memory? fdopen failed");
> +#ifdef __TANDEM
> +   setbuf(stream,0);

My man page tells me the second arg is a pointer,
so we'd prefer NULL instead?

Thanks,
Stefan

> +#endif
> return stream;
>  }
>
> --
> 2.16.0.31.gf1a482c
>

Re: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Stefan Beller]

On Fri, Jan 19, 2018 at 9:34 AM,   wrote:
> From: "Randall S. Becker" 
>
> * remote.c: force ignoring of GCC __attribute construct not supported
>   by c99 by defining it as an empty CPP macro.
>
> Signed-off-by: Randall S. Becker 

I do not know about the __TANDEM symbol, but some research[1]
indicates it is a macro specifically for case of cross platform support:

You can use the __TANDEM symbol to increase the portability
of your programs. Enclose system-dependent source text in an
if section that uses #ifdef or #ifndef to test for the existence of the
__TANDEM symbol.

It seems to not be used outside of the NonStop port[2], so the code seems
ok. (I would have used #ifdef, as I thought this is more prevalent in our
code base and for consistency we'd rather want to use one thing only, bug
"git grep '#if defined'" proves me wrong)

However the commit message is hard to understand (say for somebody who
will find this commit in 2 years using git-blame).

Maybe:

In NonStop HPE (version X, all versions?) there is no support for the
__attribute macro. By redefining the __attribute to an empty macro, the
code compiles on NonStop HPE. Use the system specific __TANDEM
macro to guard it for just this platform.

as that will help those people in the future not having to do the research?


[1] http://h20628.www2.hp.com/km-ext/kmcsdirect/emr_na-c02121175-1.pdf
[2] https://sourceforge.net/p/predef/wiki/OperatingSystems/?version=44

Thanks,
Stefan


> ---
>  remote.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/remote.c b/remote.c
> index 4e93753e1..c18f9de7f 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -11,6 +11,10 @@
>  #include "mergesort.h"
>  #include "argv-array.h"
>
> +#if defined (__TANDEM)
> +#define __attribute(a)
> +#endif
> +
>  enum map_direction { FROM_SRC, FROM_DST };
>
>  static struct refspec s_tag_refspec = {
> --
> 2.16.0.31.gf1a482c
>

Re: [PATCH] repository: pre-initialize hash algo pointer

[Junio C Hamano]

Eric Sunshine  writes:

>> I'm still quite mystified as to why this is working on Linux and not
>> macOS, but I can only guess that compilers are just very advanced and
>> have somehow concluded that we would clearly never dereference a NULL
>> pointer, so they picked the only non-NULL value.
>
> Now that we know (due to Duy's excellent detective work[1]) that the
> trigger is files with names differing only in case on case-insensitive
> filesystems, the commit message can be updated appropriately.

Thanks.  Let me apply the following and do a 2.16.1, hopefully by
the end of day or mid tomorrow at the latest.  Test to protect the
fix can come as a separate follow-up patch.

-- >8 --
Subject: [PATCH] repository: pre-initialize hash algo pointer

There are various git subcommands (among them, clone) which don't set up
the repository (that is, they lack RUN_SETUP or RUN_SETUP_GENTLY) but
end up needing to have information about the hash algorithm in use.
Because the hash algorithm is part of struct repository and it's only
initialized in repository setup, we can end up dereferencing a NULL
pointer in some cases if we call one of these subcommands and look up
the empty blob or empty tree values.

A "git clone" of a project that has two paths that differ only in
case suffers from this if it is run on a case insensitive platform.
When the command attempts to check out one of these two paths after
checking out the other one, the checkout codepath needs to see if
the version that is already on the filesystem (which should not
happen if the FS were case sensitive), and it needs to exercise the
hashing code.

In the future, we can add a command line option for this or read it
from the configuration, but until we're ready to expose that
functionality to the user, simply initialize the repository
structure to use the current hash algorithm, SHA-1.

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

diff --git a/repository.c b/repository.c
index 998413b8bb..f66fcb1342 100644
--- a/repository.c
+++ b/repository.c
@@ -5,7 +5,7 @@
 
 /* The main repository */
 static struct repository the_repo = {
-   NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, _index, NULL, 
0, 0
+   NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, _index, 
_algos[GIT_HASH_SHA1], 0, 0
 };
 struct repository *the_repository = _repo;
 
-- 
2.16.0-204-gc262421c89

Re: [PATCH v2] packfile: use get_be64() for large offsets

[Junio C Hamano]

SZEDER Gábor  writes:

> Junio,
> ...
> This patch can't be applied to 'maint' currently at 3013dff86 (Prepare
> for 2.15.2, 2017-12-06), as it is in case of 'ds/use-get-be64',
> because 'maint' doesn't have get_be64() yet (b2e39d006 (bswap: add 64
> bit endianness helper get_be64, 2017-09-22)).

Thanks for stopping me.  

I do not always get to test anything not in 'next' in isolation
before I actually try to merge it there (at which time I would have
noticed), and you caught me in this one doing a short-cut "just
queue it somewhere and throw it in 'pu'".

Re: [PATCH] enable core.fsyncObjectFiles by default

[Junio C Hamano]

Christoph Hellwig  writes:

> [adding Chris to the Cc list - this is about the awful ext3 data=ordered
> behavior of syncing the whole file system data and metadata on each
> fsync]
>
> On Wed, Jan 17, 2018 at 03:57:53PM -0800, Linus Torvalds wrote:
>> On Wed, Jan 17, 2018 at 3:52 PM, Theodore Ts'o  wrote:
>> >
>> > Well, let's be fair; this is something *ext3* got wrong, and it was
>> > the default file system back them.
>> 
>> I'm pretty sure reiserfs and btrfs did too..
>
> I'm pretty sure btrfs never did, and reiserfs at least looks like
> it currently doesn't but I'd have to dig into history to check if
> it ever did.

So..., is it fair to say that the one you sent in

  https://public-inbox.org/git/20180117193510.ga30...@lst.de/

is the best variant we have seen in this thread so far?  I'll keep
that in my inbox so that I do not forget, but I think we would want
to deal with a hotfix for 2.16 on case insensitive platforms before
this topic.

Thanks.

Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision

[Jacob Keller]

On Fri, Jan 19, 2018 at 10:55 AM, Phillip Wood
 wrote:
> On 19/01/18 12:24, Phillip Wood wrote:
>>
>> On 18/01/18 15:35, Johannes Schindelin wrote:
>>>
>>> Internally, the `label ` command creates the ref
>>> `refs/rewritten/`. This makes it possible to work with the labeled
>>> revisions interactively, or in a scripted fashion (e.g. via the todo
>>> list command `exec`).
>>
>> If a user has two work trees and runs a rebase in each with the same
>> label name, they'll clobber each other. I'd suggest storing them under
>> refs/rewritten/ instead. If the user
>> tries to rebase a second worktree with the same detached HEAD as an
>> existing rebase then refuse to start.
>>
>
> Ah this isn't a concern after all as patch 5 makes refs/rewritten local
> to the worktree. Perhaps you could move that part of patch 5 here or add
> a note to the commit message that it will become worktree local later in
> the series
>
> Best Wishes
>
> Phillip

I'd rather it be included here as well.

Thanks,
Jake

Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision

[Phillip Wood]

On 19/01/18 12:24, Phillip Wood wrote:
> 
> On 18/01/18 15:35, Johannes Schindelin wrote:
>>
>> Internally, the `label ` command creates the ref
>> `refs/rewritten/`. This makes it possible to work with the labeled
>> revisions interactively, or in a scripted fashion (e.g. via the todo
>> list command `exec`).
> 
> If a user has two work trees and runs a rebase in each with the same
> label name, they'll clobber each other. I'd suggest storing them under
> refs/rewritten/ instead. If the user
> tries to rebase a second worktree with the same detached HEAD as an
> existing rebase then refuse to start.
> 

Ah this isn't a concern after all as patch 5 makes refs/rewritten local
to the worktree. Perhaps you could move that part of patch 5 here or add
a note to the commit message that it will become worktree local later in
the series

Best Wishes

Phillip

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Junio C Hamano]

Jeff King  writes:

> I also think %(deltabase) does make sense for anything that points to an
> object. I suspect it's not all that _useful_ for for-each-ref, but that
> doesn't mean we can't return the sensible thing if somebody asks for it.

This may not be a new issue (or any issue at all), but is the
ability to learn deltabase make any sense in the first place?

What should the code do when an object has three copies in the
repo, i.e. one as a base object (or a loose one), another as a
delta against an object, and the third one as a delta against
a different object?

Re: should any build system legitimately change any tracked files?

[Robin H. Johnson]

On Fri, Jan 19, 2018 at 12:51:52PM -0500, Robert P. J. Day wrote:
> 
>   just finished teaching a couple git courses and, after class, a
> student came up and described a rather weird problem -- in short:
> 
>   1) before build, "git diff" shows nothing
>   2) do the standard build
>   3) suddenly, "git diff" shows some changes
> 
> that's all the info i was given, but it *seems* clear that the build
> process itself was making changes to one or more tracked files.
> 
>   technically, i guess one can design a build system to do pretty
> much anything, but is it fair to say that this is a really poor design
> decision? admittedly, this isn't specifically a git question, but i'm
> open to opinions on something that strikes me as a bad idea.
I have seen what you describe, but it had a good cause:
1. The source repo contained some intermediate generated source, 
   eg foo.x -> foo.c -> foo.o
2. The output of the tool that did foo.a -> foo.c differed due to some
   factor on the system (different version, different config in /etc etc).
3. The initial checkout caused the mtime of foo.c to be just older
   newer than foo.x, so the build system decided to regen foo.c.
4. (optional) The makefile had conditional rules to skip the regen if the tool 
was
   not present.

Until the tool output changed, even if the file was regenerated, it was
identical, so it didn't show up in diff.

What are the possible mistakes here?
- The intermediate source possibly should not be committed [depending on
  the tool, this isn't always an option]
- The build system scripts (makefile etc) contains a mistake.
- Some final (non-intermediate/non-source) file was committed.

I've seen similar patterns for GNU Bison, autoconf, and lots of other
tools. 

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robb...@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136


signature.asc
Description: Digital signature

Re: should any build system legitimately change any tracked files?

[Theodore Ts'o]

On Fri, Jan 19, 2018 at 12:51:52PM -0500, Robert P. J. Day wrote:
> that's all the info i was given, but it *seems* clear that the build
> process itself was making changes to one or more tracked files.
> 
>   technically, i guess one can design a build system to do pretty
> much anything, but is it fair to say that this is a really poor design
> decision? admittedly, this isn't specifically a git question, but i'm
> open to opinions on something that strikes me as a bad idea.

I agree that in general it's a bad idea.  I can see how it happens,
though, which is because two things come into tension:

1) The general desire not to check in generated files into the git
repository --- including configure files generated by autoconf,
Makefiles generated by automake, libtool files, etc.

2) Wanting not to give users trying to build from source a non-hostile
experience.  Unfortunately autoconf/automake/libtool systems are
notorious for not having a stable interface, such that if you have the
wrong or outdated version of the tools, the results of generating the
configure, Makefile, etc., using a different version than what the
developer used well, your results may vary.

What I do is use "Maintainer mode" which means that the generated
files are *not* automatically rebuilt by the build system unless you
configure with --enable-maintainer-mode, and then I *do* check in the
generated files into git.  That way I can run with
--enable-maintainer-mode, and check in updates to Makefile, configure,
etc., as necessary when the input files change, but that way, end
users don't have to worry getting ambushed by version skew caused by
using an old (or unexpectedly newer) version of the
autoconf/autoconf/libtool tools.

Heck, I even have had config.guess/config.sub change on me in
incompatible ways(*), so I ship my own version and don't enable a blind
update of those files from the upstream FSF sources --- mainly because
I don't trust them to preserve a stable interface.  Better that I
manually pull them into the repo, and test them before I do a public
release.

- Ted

(*) Although to be fair it's been years since I've been screwed in
this fashion.  But once bitten, twice shy

Re: [PATCH v3 0/3] fixes for split index mode

[Junio C Hamano]

Thomas Gummerer  writes:

> Friendly ping on this series now that 2.16 is out :) Is there anything
> in this series (up to 3/3, 4/3 can be dropped now that Duy fixed it in
> a nicer way) that still needs updating?  It fixes a few bugs in split
> index mode with submodules/worktrees, so it would be nice to get this
> reviewed/merged.

I was wondering about the same thing.  Especially it wasn't very
clear to me what Duy's replacement was meant to replace and how well
it was supposed to work with the rest of your series.

Let's drop 4/3 and queue 1-3/3 for now.

Re: Segmentation fault on clone

[Stephen M. McQuay]

On Fri, Jan 19, 2018 at 01:22:04PM -0500, Todd Zullinger wrote:

This seems likely to be the same segfault as:

https://public-inbox.org/git/calwadsgfb10f5+nofn-phct4z1skwmcvshn8kokcycm0v6k...@mail.gmail.com/

There's a patch in that thread.


I've verified that the patch fixes my case.

I'll back off to 2.15.x and wait for that (or similar) to land.

Thanks for the pointer,

--
Stephen


signature.asc
Description: PGP signature

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

[Christian Couder]

On Tue, Jan 16, 2018 at 2:46 AM, 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.

You might want to say that this provides a significant code reduction
which shows that the mru API is not a very useful abstraction anymore.

> Another discussion, here
> (https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/)
> was on what has to be done with the next pointer of packed git type

I think using "pointer to a 'struct packed_git'" instead of "pointer
of packed git type" would be clearer here, but anyway see below.

> inside the
> packed_git structure. It can be removed _given_ that no one needs to
> access the list in order and can be sent as another patch.

I don't think it's worth pointing to a discussion about a future
improvement in the commit message. You could perhaps even remove all
the above paragraph as this commit is valuable and self contained
enough by itself.

> ---
> Changes in v2:
> - Add a move to front function to the list API.
> - Remove memory leak.
> - Remove redundant remove operations on the list.
>
> The commit has been built on top of ot/mru-on-list branch.

Nice!

>  Makefile   |  1 -
>  builtin/pack-objects.c | 12 ++--
>  cache.h|  9 +
>  list.h |  7 +++
>  mru.c  | 27 ---
>  mru.h  | 40 
>  packfile.c | 18 +-
>  sha1_file.c|  1 -
>  8 files changed, 27 insertions(+), 88 deletions(-)
>  delete mode 100644 mru.c
>  delete mode 100644 mru.h

Very nice!

[...]

> @@ -1030,8 +1029,9 @@ static int want_object_in_pack(const unsigned char 
> *sha1,
> *found_pack = p;
> }
> want = want_found_object(exclude, p);
> -   if (!exclude && want > 0)
> -   mru_mark(_git_mru, entry);
> +   if (!exclude && want > 0) {
> +   list_move_to_front(>mru, _git_mru);
> +   }

Style: we usually remove brackets when there is one line after the
if(...) line. (See the 2 lines that you delete.)

Otherwise the patch looks good to me.

Thanks,
Christian.

Re: Segmentation fault on clone

[Todd Zullinger]

Hi Stephen,

Stephen M. McQuay wrote:
> I submitted a bug against the brew project when git
> version 2.16.0 started segfaulting:
> 
> https://github.com/Homebrew/homebrew-core/issues/23045#issuecomment-358891009

This seems likely to be the same segfault as:

https://public-inbox.org/git/calwadsgfb10f5+nofn-phct4z1skwmcvshn8kokcycm0v6k...@mail.gmail.com/

There's a patch in that thread.

-- 
Todd
~~
The man who is a pessimist before forty-eight knows too much; the man
who is an optimist after forty-eight knows too little.
-- Mark Twain

Segmentation fault on clone

[Stephen M. McQuay]

I submitted a bug against the brew project when git version 2.16.0 
started segfaulting:


https://github.com/Homebrew/homebrew-core/issues/23045#issuecomment-358891009

I've built git from master from https://github.com/git/git and was able 
to reproduce (no surprise, the top commit claims it's 2.16.0), and also 
built from the `next` branch which yields similar results.


I'm on latest customer release of MacOS:

   $ uname -a
   Darwin smm.local 17.3.0 Darwin Kernel Version 17.3.0: Thu Nov  9 18:09:22 
PST 2017; root:xnu-4570.31.3~1/RELEASE_X86_64 x86_64

   $ sw_vers
   ProductName:Mac OS X
   ProductVersion: 10.13.2
   BuildVersion:   17C205

The clone url in question: g...@github.com:gdamore/tcell.git

Any other information that would be helpful?

Respectfully,

--
Stephen


signature.asc
Description: PGP signature

RE: should any build system legitimately change any tracked files?

[Randall S. Becker]

On January 19, 2018 12:52 PM, Robert P. J. Day wrote:
>   just finished teaching a couple git courses and, after class, a student
came
> up and described a rather weird problem -- in short:
> 
>   1) before build, "git diff" shows nothing
>   2) do the standard build
>   3) suddenly, "git diff" shows some changes
> 
> that's all the info i was given, but it *seems* clear that the build
process itself
> was making changes to one or more tracked files.
> 
>   technically, i guess one can design a build system to do pretty much
> anything, but is it fair to say that this is a really poor design
decision?
> admittedly, this isn't specifically a git question, but i'm open to
opinions on
> something that strikes me as a bad idea.

Depends what you're up to.  Changing the source repository content is
probably bad. Adding tags may not be. Also, updating a separate repository
to include build information (a.k.a dependency tracking between source and
object commits) can be very useful for managing production builds and
environments.

Cheers,
Randall

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

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Fri, Jan 19, 2018 at 6:22 PM, Оля Тележная  wrote:
> 2018-01-19 20:14 GMT+03:00 Christian Couder :
>> On Thu, Jan 18, 2018 at 7:20 AM, Оля Тележная  
>> wrote:

>>> And another thoughts here - we were thinking about creating format.h
>>> but decided not to move forward with it, and now we are suffering
>>> because of it. Can I create it right now or the history of commits
>>> would be too dirty because of it?
>>
>> It would also make it difficult to refactor your patch series if there
>> is a big move or renaming in the middle.
>>
>>> Also, do you mean just renaming of
>>> ref-filter? I was thinking that I need to put formatting-related logic
>>> to another file and leave all other stuff in ref-filter.
>>
>> Yeah, you can do both a move and a renaming.
>
> Thanks for a response! That thought is not clear enough for me. Do you
> want me to split ref-filter into 2 files (one is for formatting only
> called format and other one is for anything else still called
> ref-filter) - here is a second question by the way, do I need to
> create only format.h (and leave all realizations in ref-filter.c), or
> I also need to create format.c. Or, just to rename ref-filter into
> format and that's all.

Just renaming ref-filter into format (including the filenames) will
probably be enough, but it's also possible that it will make more
sense to keep some code only relevant to ref filtering into
ref-filter.{c,h}. We will be in a better position to decide what we
should do when the migration is finished.

Re: [PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes

[René Scharfe]

Am 18.01.2018 um 23:40 schrieb SZEDER Gábor:
> On Thu, Jan 18, 2018 at 10:40 PM, René Scharfe  wrote:
>> Am 16.01.2018 um 18:11 schrieb SZEDER Gábor:
>>> Unfortunately, most of the changes coming from 'strbuf.cocci' don't
>>> make any sense, they appear to be the mis-application of the "use
>>> strbuf_addstr() instead of strbuf_addf() to add a single string" rule:
>>>
>>> - strbuf_addf(_repo, "%d", counter);
>>> + strbuf_addstr(_repo, counter);
>>>
>>> It seems that those rules need some refinement, but I have no idea
>>> about Coccinelle and this is not the time for me to dig deeper.
>>>
>>> What makes all this weird is that running 'make coccicheck' on my own
>>> machine doesn't produce any of these additional proposed changes, just
>>> like at René's.  Can it be related to differing Coccinelle versions?
>>> Travis CI installs 1.0.0~rc19.deb-3; I have 1.0.4.deb-2.
>>
>> The version difference may explain it, but I couldn't find a matching
>> bugfix in http://coccinelle.lip6.fr/distrib/changes.html when I just
>> skimmed it.  I wonder if the following patch could make a difference:
> 
> Yes, it does, now all those nonsense suggestions are gone on Travis CI.

I would have expected matching a literal "%s" to be easier than
dissecting that (admittedly simple) format string, but if it all works
out fine then I'm not complaining. :)  Sent the patch again properly.

>https://travis-ci.org/szeder/git/jobs/330572425#L713
> 
> Those "memmove() -> MOVE_ARRAY" suggestions are still there, of course.

They look valid and nice to have in that report.  I wonder why we don't
get them locally, though.  Are you going to submit them as a patch?

(NB: The patches generated by coccicheck apply with "patch -p0", unlike
 those generated by git diff and friends.)

Thanks,
René

should any build system legitimately change any tracked files?

[Robert P. J. Day]

  just finished teaching a couple git courses and, after class, a
student came up and described a rather weird problem -- in short:

  1) before build, "git diff" shows nothing
  2) do the standard build
  3) suddenly, "git diff" shows some changes

that's all the info i was given, but it *seems* clear that the build
process itself was making changes to one or more tracked files.

  technically, i guess one can design a build system to do pretty
much anything, but is it fair to say that this is a really poor design
decision? admittedly, this isn't specifically a git question, but i'm
open to opinions on something that strikes me as a bad idea.

rday

[PATCH v2 3/6] Define config options required for the HPE NonStop NSX and NSE platforms

[randall . s . becker]

From: "Randall S. Becker" 

* config.mak.uname: upgrade old options to currently supported
  NonStop operating system versions (J06.21 and L17.xx).

Signed-off-by: Randall S. Becker 
---
 config.mak.uname | 29 +
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/config.mak.uname b/config.mak.uname
index 685a80d13..d9f8d57e3 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -428,27 +428,37 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
# INLINE='' would just replace one set of warnings with another and
# still not compile in c89 mode, due to non-const array initializations.
CC = cc -c99
+   # Build down-rev compatible objects that don't use our new getopt_long.
+   ifeq ($(uname_R).$(uname_V),J06.21)
+   CC += -WRVU=J06.20
+   endif
+   ifeq ($(uname_R).$(uname_V),L17.02)
+   CC += -WRVU=L16.05
+   endif
# Disable all optimization, seems to result in bad code, with -O or -O2
# or even -O1 (default), /usr/local/libexec/git-core/git-pack-objects
# abends on "git push". Needs more investigation.
-   CFLAGS = -g -O0
+   CFLAGS = -g -O0 -Winline
# We'd want it to be here.
prefix = /usr/local
# Our's are in ${prefix}/bin (perl might also be in /usr/bin/perl).
-   PERL_PATH = ${prefix}/bin/perl
-   PYTHON_PATH = ${prefix}/bin/python
-
+   PERL_PATH = /usr/bin/perl
+   PYTHON_PATH = /usr/bin/python
+   RM = /bin/rm -f
# As detected by './configure'.
# Missdetected, hence commented out, see below.
#NO_CURL = YesPlease
# Added manually, see above.
NEEDS_SSL_WITH_CURL = YesPlease
+   NEEDS_CRYPTO_WITH_SSL = YesPlease
+   HAVE_DEV_TTY = YesPlease
HAVE_LIBCHARSET_H = YesPlease
HAVE_STRINGS_H = YesPlease
NEEDS_LIBICONV = YesPlease
NEEDS_LIBINTL_BEFORE_LIBICONV = YesPlease
NO_SYS_SELECT_H = UnfortunatelyYes
NO_D_TYPE_IN_DIRENT = YesPlease
+   NO_GETTEXT = YesPlease
NO_HSTRERROR = YesPlease
NO_STRCASESTR = YesPlease
NO_MEMMEM = YesPlease
@@ -458,8 +468,13 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
NO_MKDTEMP = YesPlease
# Currently libiconv-1.9.1.
OLD_ICONV = UnfortunatelyYes
-   NO_REGEX = YesPlease
+   NO_REGEX=NeedsStartEnd
NO_PTHREADS = UnfortunatelyYes
+   ifdef NO_PTHREADS
+   else # WIP, use Posix User Threads
+   PTHREAD_CFLAGS = -D_PUT_MODEL_ -I/usr/include
+   PTHREAD_LIBS = -lput
+   endif
 
# Not detected (nor checked for) by './configure'.
# We don't have SA_RESTART on NonStop, unfortunalety.
@@ -477,9 +492,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
# RFE 10-120912-4693 submitted to HP NonStop development.
NO_SETITIMER = UnfortunatelyYes
SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
-   SHELL_PATH = /usr/local/bin/bash
-   # as of H06.25/J06.14, we might better use this
-   #SHELL_PATH = /usr/coreutils/bin/bash
+   SHELL_PATH = /usr/coreutils/bin/bash
 endif
 ifneq (,$(findstring MINGW,$(uname_S)))
pathsep = ;
-- 
2.16.0.31.gf1a482c

[PATCH v2 4/6] Force test suite traps to be cleared for NonStop ksh

[randall . s . becker]

From: "Randall S. Becker" 

* t/lib-git-daemon.sh: fix incompatibilities with ksh traps not being
  cleared automatically on platform. This caused tests to seem to fail
  while actually succeeding.

Signed-off-by: Randall S. Becker 
---
 t/lib-git-daemon.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 987d40680..955beecd9 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -68,6 +68,7 @@ start_git_daemon() {
test_skip_or_die $GIT_TEST_GIT_DAEMON \
"git daemon failed to start"
fi
+   trap '' EXIT
 }
 
 stop_git_daemon() {
@@ -89,4 +90,6 @@ stop_git_daemon() {
fi
GIT_DAEMON_PID=
rm -f git_daemon_output
+
+   trap '' EXIT
 }
-- 
2.16.0.31.gf1a482c

[PATCH v2 6/6] Add intptr_t and uintptr_t to regcomp.c for NonStop platform.

[randall . s . becker]

From: "Randall S. Becker" 

* compat/regex/regcomp.c: fix missing intptr_t on NonStop. This is
  done because git-compat-util.h cannot be cleanly included into
  this file without additional compile errors.

Signed-off-by: Randall S. Becker 
---
 compat/regex/regcomp.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index 51cd60baa..8bb4c966d 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c
@@ -17,6 +17,14 @@
License along with the GNU C Library; if not, see
.  */
 
+#if defined __TANDEM
+/* This is currently duplicated from git-compat-utils.h */
+#ifdef NO_INTPTR_T
+typedef long intptr_t;
+typedef unsigned long uintptr_t;
+#endif
+#endif
+
 static reg_errcode_t re_compile_internal (regex_t *preg, const char * pattern,
  size_t length, reg_syntax_t syntax);
 static void re_compile_fastmap_iter (regex_t *bufp,
-- 
2.16.0.31.gf1a482c

[PATCH v2 2/6] Add tar extract install options override in installation processing.

[randall . s . becker]

From: "Randall S. Becker" 

* Makefile: Add TAR_EXTRACT_OPTIONS to allow platform options to be
  specified if needed. The default is xof.

Signed-off-by: Randall S. Becker 
---
 Makefile | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 1a9b23b67..040e9eacd 100644
--- a/Makefile
+++ b/Makefile
@@ -429,6 +429,9 @@ all::
 # running the test scripts (e.g., bash has better support for "set -x"
 # tracing).
 #
+# Define TAR_EXTRACT_OPTIONS if you want to change the default behaviour
+# from xvf to something else during installation.
+#
 # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
 # which the built Git will run (for instance "x86_64").
 
@@ -452,6 +455,7 @@ LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
 STRIP ?= strip
+TAR_EXTRACT_OPTIONS = xof
 
 # Create as necessary, replace existing, make ranlib unneeded.
 ARFLAGS = rcs
@@ -2569,7 +2573,7 @@ install: all
 ifndef NO_GETTEXT
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
(cd po/build/locale && $(TAR) cf - .) | \
-   (cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) xof -)
+   (cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) 
$(TAR_EXTRACT_OPTIONS) -)
 endif
 ifndef NO_PERL
$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
-- 
2.16.0.31.gf1a482c

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

[randall . s . becker]

From: "Randall S. Becker" 

* wrapper.c: called setbuf(stream,0) to force pipe flushes not enabled by
  default on the NonStop platform.

Signed-off-by: Randall S. Becker 
---
 wrapper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/wrapper.c b/wrapper.c
index d20356a77..671cbb4b4 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
FILE *stream = fdopen(fd, mode);
if (stream == NULL)
die_errno("Out of memory? fdopen failed");
+#ifdef __TANDEM
+   setbuf(stream,0);
+#endif
return stream;
 }
 
-- 
2.16.0.31.gf1a482c

[PATCH v2 5/6] Bring NonStop platform definitions up to date in git-compat-util.h

[randall . s . becker]

From: "Randall S. Becker" 

* git-compat-util.h: add correct FLOSS definitions to allow correct
  emulation of non-platform behaviour. Add NSIG definition that is
  not explicitly supplied in signal.h on platform.

Signed-off-by: Randall S. Becker 
---
 git-compat-util.h | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 68b2ad531..1e13f050a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -378,6 +378,21 @@ static inline char *git_find_last_dir_sep(const char *path)
 #define find_last_dir_sep git_find_last_dir_sep
 #endif
 
+#ifdef __TANDEM
+#if !defined(_THREAD_SUPPORT_FUNCTIONS) && !defined(_PUT_MODEL_)
+/* #include  */
+/* #include  */
+#endif
+#include 
+#include 
+#if ! defined NSIG
+/* NonStop NSE and NSX do not provide NSIG. SIGGUARDIAN(99) is the highest
+   known, by detective work using kill -l as a list of all signals
+   instead of signal.h where it should be. */
+# define NSIG 100
+#endif
+#endif
+
 #if defined(__HP_cc) && (__HP_cc >= 61000)
 #define NORETURN __attribute__((noreturn))
 #define NORETURN_PTR
-- 
2.16.0.31.gf1a482c

[PATCH v2 0/6] Cover Letter for NonStop Port Patches

[randall . s . becker]

From: "Randall S. Becker" 

This is the second attempt at submission of the NonStop port to
the git team. This package is split by file but should be applied
atomically.

I am not happy with the change in compat/regex/regcomp.c and figure
this might change with feedback.

Sincerely,
Randall

Randall S. Becker (7):
  Force pipes to flush immediately on NonStop platform
  Bypass GCC attributes on NonStop platform where used.
  Add tar extract install options override in installation processing.
  Define config options required for the HPE NonStop NSX and NSE
platforms
  Force test suite traps to be cleared for NonStop ksh
  Bring NonStop platform definitions up to date in git-compat-util.h
  Add intptr_t and uintptr_t to regcomp.c for NonStop platform.

 Makefile   |  6 +-
 compat/regex/regcomp.c |  8 
 config.mak.uname   | 29 +
 git-compat-util.h  | 15 +++
 remote.c   |  4 
 t/lib-git-daemon.sh|  3 +++
 wrapper.c  |  3 +++
 7 files changed, 59 insertions(+), 9 deletions(-)

-- 
2.16.0.31.gf1a482c

[PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[randall . s . becker]

From: "Randall S. Becker" 

* remote.c: force ignoring of GCC __attribute construct not supported
  by c99 by defining it as an empty CPP macro.

Signed-off-by: Randall S. Becker 
---
 remote.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/remote.c b/remote.c
index 4e93753e1..c18f9de7f 100644
--- a/remote.c
+++ b/remote.c
@@ -11,6 +11,10 @@
 #include "mergesort.h"
 #include "argv-array.h"
 
+#if defined (__TANDEM)
+#define __attribute(a)
+#endif
+
 enum map_direction { FROM_SRC, FROM_DST };
 
 static struct refspec s_tag_refspec = {
-- 
2.16.0.31.gf1a482c

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Fri, Jan 19, 2018 at 6:23 PM, Jeff King  wrote:
> On Fri, Jan 19, 2018 at 06:14:56PM +0100, Christian Couder wrote:
>
>> > Let's discuss, what behavior we are waiting for
>> > when atom seems useless for the command. Die or ignore?
>>
>> We could alternatively just emit a warning, but it looks like there
>> are a lot of die() calls already in ref-filter.c, so I would suggest
>> die().
>
> I actually think it makes sense to just expand nonsense into the empty
> string, for two reasons:
>
>   1. That's what ref-filter already does for the existing cases. For
>  example, try:
>
>git for-each-ref --format='%(objecttype) %(authordate)'
>
>  and you will see that the annotated tags just get a blank author
>  field.
>
>   2. I think we may end up with a case where we feed multiple objects
>  via --batch-check, and the format is only nonsense for _some_ of
>  them. E.g., I envision a world where you can do:
>
>git cat-file --batch-check='%(objecttype) %(refname)' <<-\EOF
>master
>12345abcde12345abcde12345abcde12345abcde
>EOF
>
>  and get output like:
>
>commit refs/heads/master
>commit
>
>  (i.e., if we would remember the refname discovered during the name
>  resolution, we could still report it). It would be annoying if the
>  second line caused us to die().

Yeah, ok, that makes sense.

>> > And, which
>> > atoms are useless (as I understand, "rest" and "deltabase" from
>> > cat-file are useless for all ref-filter users, so the question is
>> > about - am I right in it, and about ref-filter atoms for cat-file).
>>
>> For now and I think until the migration process is finished, you could
>> just die() in case of any atom not already supported by the command.
>
> I'm OK with dying in the interim if it's easier, though I suspect it is
> not much extra work to just expand to the empty string in such cases. If
> that's where we want to end up eventually, it may be worth going
> straight there.
>
> I also think %(deltabase) does make sense for anything that points to an
> object. I suspect it's not all that _useful_ for for-each-ref, but that
> doesn't mean we can't return the sensible thing if somebody asks for it.
>
> I agree that %(rest) probably doesn't make any sense for a caller which
> isn't parsing input.

Yeah, ok.

Re: git 2.16.0 segfaults on clone of specific repo

[Todd Zullinger]

Eric Sunshine wrote:
> Nice detective work. This particular manifestation is caught by the
> following test which fails without brian's patch on MacOS (and
> presumably Windows) and succeeds with it. On Linux and BSD, it will of
> course succeed always, so I'm not sure how much practical value it
> has.

The CASE_INSENSITIVE_FS prereq could be used to avoid
running the test on systems where it won't provide much
value, couldn't it?

> --- >8 ---
> hex2oct() {
>   perl -ne 'printf "\\%03o", hex for /../g'
> }
> 
> test_expect_success 'clone on case-insensitive fs' '
>   o=$(git hash-object -w --stdint=$(printf "100644 X\0${o}100644 x\0${o}" |
>  git hash-object -w -t tree --stdin) &&
>   c=$(git commit-tree -m bogus $t) &&
>   git update-ref refs/heads/bogus $c &&
>   git clone -b bogus . bogus
> '
> --- >8 ---
> 
> (hex2oct lifted from t1007/t1450)

-- 
Todd
~~
Money frees you from doing things you dislike. Since I dislike doing
nearly everything, money is handy.
-- Groucho Marx

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Jeff King]

On Fri, Jan 19, 2018 at 06:14:56PM +0100, Christian Couder wrote:

> > Let's discuss, what behavior we are waiting for
> > when atom seems useless for the command. Die or ignore?
> 
> We could alternatively just emit a warning, but it looks like there
> are a lot of die() calls already in ref-filter.c, so I would suggest
> die().

I actually think it makes sense to just expand nonsense into the empty
string, for two reasons:

  1. That's what ref-filter already does for the existing cases. For
 example, try:

   git for-each-ref --format='%(objecttype) %(authordate)'

 and you will see that the annotated tags just get a blank author
 field.

  2. I think we may end up with a case where we feed multiple objects
 via --batch-check, and the format is only nonsense for _some_ of
 them. E.g., I envision a world where you can do:

   git cat-file --batch-check='%(objecttype) %(refname)' <<-\EOF
   master
   12345abcde12345abcde12345abcde12345abcde
   EOF

 and get output like:

   commit refs/heads/master
   commit

 (i.e., if we would remember the refname discovered during the name
 resolution, we could still report it). It would be annoying if the
 second line caused us to die().

> > And, which
> > atoms are useless (as I understand, "rest" and "deltabase" from
> > cat-file are useless for all ref-filter users, so the question is
> > about - am I right in it, and about ref-filter atoms for cat-file).
> 
> For now and I think until the migration process is finished, you could
> just die() in case of any atom not already supported by the command.

I'm OK with dying in the interim if it's easier, though I suspect it is
not much extra work to just expand to the empty string in such cases. If
that's where we want to end up eventually, it may be worth going
straight there.

I also think %(deltabase) does make sense for anything that points to an
object. I suspect it's not all that _useful_ for for-each-ref, but that
doesn't mean we can't return the sensible thing if somebody asks for it.

I agree that %(rest) probably doesn't make any sense for a caller which
isn't parsing input.

-Peff

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Оля Тележная]

2018-01-19 20:14 GMT+03:00 Christian Couder :
> On Thu, Jan 18, 2018 at 7:20 AM, Оля Тележная  
> wrote:
>> 2018-01-18 1:39 GMT+03:00 Christian Couder :
>>> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King  wrote:
 On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:

> > IOW, the progression I'd expect in a series like this is:
> >
> >   1. Teach ref-filter.c to support everything that cat-file can do.
> >
> >   2. Convert cat-file to use ref-filter.c.
>
> I agree, I even made this and it's working fine:
> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>>>
>>> (Nit: it looks like the above link does not work any more, but it
>>> seems that you are talking about the last patch on the catfile
>>> branch.)
>>>
> But I decided not to add that to patch because I expand the
> functionality of several commands (not only cat-file and
> for-each-ref), and I need to support all new functionality in a proper
> way, make these error messages, test everything and - the hardest one
> - support many new commands for cat-file. As I understand, it is not
> possible unless we finally move to ref-filter and print results also
> there. Oh, and I also need to rewrite docs in that case. And I decided
> to apply this in another patch. But, please, say your opinion, maybe
> we could do that here in some way.

 Yeah, I agree that it will cause changes to other users of ref-filter,
 and you'd need to deal with documentation and tests there. But I think
 we're going to have to do those things eventually (since supporting
 those extra fields everywhere is part of the point of the project). And
 by doing them now, I think it can make the transition for cat-file a lot
 simpler, because we never have to puzzle over this intermediate state
 where only some of the atoms are valid for cat-file.
>>>
>>> I agree that you will have to deal with documentation and tests at one
>>> point and that it could be a good idea to do that now.
>>>
>>> I wonder if it is possible to add atoms one by one into ref-filter and
>>> to add tests and documentation at the same time, instead of merging
>>> cat-file atoms with ref-filter atoms in one big step.
>>>
>>> When all the cat-file atoms have been added to ref-filter's
>>> valid_atom, maybe you could add ref-filter's atoms to cat-file's
>>> valid_cat_file_atom one by one and add tests and documentation at the
>>> same time.
>>>
>>> And then when ref-filter's valid_atom and cat-file's
>>> valid_cat_file_atom are identical you can remove cat-file's
>>> valid_cat_file_atom and maybe after that rename "ref-filter" to
>>> "format".
>>
>> I think it's important to finish migrating process at first. I mean,
>> now we are preparing and collecting everything in ref-filter, but we
>> make resulting string and print still in cat-file. And I am not sure,
>> but maybe it will not be possible to start using new atoms in cat-file
>> while some part of logic still differs.
>
> Ok, you can finish the migration process then.
>
>> And another thoughts here - we were thinking about creating format.h
>> but decided not to move forward with it, and now we are suffering
>> because of it. Can I create it right now or the history of commits
>> would be too dirty because of it?
>
> It would also make it difficult to refactor your patch series if there
> is a big move or renaming in the middle.
>
>> Also, do you mean just renaming of
>> ref-filter? I was thinking that I need to put formatting-related logic
>> to another file and leave all other stuff in ref-filter.
>
> Yeah, you can do both a move and a renaming.

Thanks for a response! That thought is not clear enough for me. Do you
want me to split ref-filter into 2 files (one is for formatting only
called format and other one is for anything else still called
ref-filter) - here is a second question by the way, do I need to
create only format.h (and leave all realizations in ref-filter.c), or
I also need to create format.c. Or, just to rename ref-filter into
format and that's all.

>
>> Anyway, your suggested steps looks good, and I am planning to
>> implement them later.
>
> Ok.
>
>> Let's discuss, what behavior we are waiting for
>> when atom seems useless for the command. Die or ignore?
>
> We could alternatively just emit a warning, but it looks like there
> are a lot of die() calls already in ref-filter.c, so I would suggest
> die().
>
>> And, which
>> atoms are useless (as I understand, "rest" and "deltabase" from
>> cat-file are useless for all ref-filter users, so the question is
>> about - am I right in it, and about ref-filter atoms for cat-file).
>
> For now and I think until the migration process is finished, you could
> just die() in case of any atom not already supported by the command.
>
>> I have never written any tests 

( United Nations Compensation Unit )

[UNO.ORG]

United Nations Compensation Unit, Emergency Relief Coordinator, United Nations. 




Congratulations Beneficiary,




We have been having a meeting for quit sometime now and we just came to a 
logical conclusion 72 hours ago in affiliation with the World Bank president. 
Your email was listed among those that are yet to receive their compensation 
payment. The United Nations in Affiliation with World Bank have agreed to 
compensate them with the sum of USD1,500, 000.00 (One Million Five Hundred 
Thousand United States Dollars) only.


For this reason, you are to receive your payment through a certified ATM MASTER 
CARD. Note, with this Master Card you can withdraw money from any part of the 
World without being disturbed or delay and please for no reason should you 
disclose your account information as your account information is not and can 
never be needed before you receive your card payment. All that is required of 
your now is to contact our 100% trust officials by the Name of Mrs. Sarah 
Ngene. Below is her contact information:


Name: Mrs. Sarah Ngene
Email: sarahng...@zenith-bank-plc.ugu.pl
Email: sarah.ng...@hotmail.com


Please ensure that you follow the directives and instructions of Mrs. Sarah 
Ngene so that within 72 hours you would have received your card payment and 
your secret pin code issued directly to you for security reasons. We apologize 
on behalf of the United Nation Organization for any delay you might have 
encountered in receiving your fund in the past. Congratulations, and I look 
forward to hear from you as soon as you confirm your payment making the world a 
better place.


Yours Faithfully,


Stephen O'Brien
Emergency Relief Coordinator, United Nations. 

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Thu, Jan 18, 2018 at 7:20 AM, Оля Тележная  wrote:
> 2018-01-18 1:39 GMT+03:00 Christian Couder :
>> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King  wrote:
>>> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>>>
 > IOW, the progression I'd expect in a series like this is:
 >
 >   1. Teach ref-filter.c to support everything that cat-file can do.
 >
 >   2. Convert cat-file to use ref-filter.c.

 I agree, I even made this and it's working fine:
 https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>>
>> (Nit: it looks like the above link does not work any more, but it
>> seems that you are talking about the last patch on the catfile
>> branch.)
>>
 But I decided not to add that to patch because I expand the
 functionality of several commands (not only cat-file and
 for-each-ref), and I need to support all new functionality in a proper
 way, make these error messages, test everything and - the hardest one
 - support many new commands for cat-file. As I understand, it is not
 possible unless we finally move to ref-filter and print results also
 there. Oh, and I also need to rewrite docs in that case. And I decided
 to apply this in another patch. But, please, say your opinion, maybe
 we could do that here in some way.
>>>
>>> Yeah, I agree that it will cause changes to other users of ref-filter,
>>> and you'd need to deal with documentation and tests there. But I think
>>> we're going to have to do those things eventually (since supporting
>>> those extra fields everywhere is part of the point of the project). And
>>> by doing them now, I think it can make the transition for cat-file a lot
>>> simpler, because we never have to puzzle over this intermediate state
>>> where only some of the atoms are valid for cat-file.
>>
>> I agree that you will have to deal with documentation and tests at one
>> point and that it could be a good idea to do that now.
>>
>> I wonder if it is possible to add atoms one by one into ref-filter and
>> to add tests and documentation at the same time, instead of merging
>> cat-file atoms with ref-filter atoms in one big step.
>>
>> When all the cat-file atoms have been added to ref-filter's
>> valid_atom, maybe you could add ref-filter's atoms to cat-file's
>> valid_cat_file_atom one by one and add tests and documentation at the
>> same time.
>>
>> And then when ref-filter's valid_atom and cat-file's
>> valid_cat_file_atom are identical you can remove cat-file's
>> valid_cat_file_atom and maybe after that rename "ref-filter" to
>> "format".
>
> I think it's important to finish migrating process at first. I mean,
> now we are preparing and collecting everything in ref-filter, but we
> make resulting string and print still in cat-file. And I am not sure,
> but maybe it will not be possible to start using new atoms in cat-file
> while some part of logic still differs.

Ok, you can finish the migration process then.

> And another thoughts here - we were thinking about creating format.h
> but decided not to move forward with it, and now we are suffering
> because of it. Can I create it right now or the history of commits
> would be too dirty because of it?

It would also make it difficult to refactor your patch series if there
is a big move or renaming in the middle.

> Also, do you mean just renaming of
> ref-filter? I was thinking that I need to put formatting-related logic
> to another file and leave all other stuff in ref-filter.

Yeah, you can do both a move and a renaming.

> Anyway, your suggested steps looks good, and I am planning to
> implement them later.

Ok.

> Let's discuss, what behavior we are waiting for
> when atom seems useless for the command. Die or ignore?

We could alternatively just emit a warning, but it looks like there
are a lot of die() calls already in ref-filter.c, so I would suggest
die().

> And, which
> atoms are useless (as I understand, "rest" and "deltabase" from
> cat-file are useless for all ref-filter users, so the question is
> about - am I right in it, and about ref-filter atoms for cat-file).

For now and I think until the migration process is finished, you could
just die() in case of any atom not already supported by the command.

> I have never written any tests and docs for Git, I will try to explore
> by myself how to do that, but if you have any special links/materials
> about it - please send them to me :)

I think that looking at the existing documentation and tests is
probably the best way to learn about it.

[PATCH] cocci: use format keyword instead of a literal string

[René Scharfe]

There's a rule in strbuf.cocci for converting trivial uses of
strbuf_addf() to strbuf_addstr() in order to simplify the code and
improve performance a bit.  Coccinelle 1.0.0~rc19.deb-3 on Travis CI
lets the "%s" in that rule match format strings like "%d" as well for
some reason, though, leading to invalid proposed patches.

Use the "format" keyword to let Coccinelle parse the format string and
match the conversion specifier with a trivial regular expression
instead.  This works fine with both Coccinelle 1.0.0~rc19.deb-3 and
1.0.4.deb-3+b3 (the current version on Debian testing).

Reported-by: SZEDER Gábor 
Tested-by: SZEDER Gábor 
Signed-off-by: Rene Scharfe 
---
 contrib/coccinelle/strbuf.cocci | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 1d580e49b0..6fe8727421 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -29,8 +29,9 @@ cocci.include_match("%" not in fmt)
 
 @@
 expression E1, E2;
+format F =~ "s";
 @@
-- strbuf_addf(E1, "%s", E2);
+- strbuf_addf(E1, "%@F@", E2);
 + strbuf_addstr(E1, E2);
 
 @@
-- 
2.16.0

Can I Trust You?

[Sgt. Britta Lopez]

Seeking your kind assistance

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

Fwd:

[A. ALLIED]

-- Forwarded message --
From: "A. ALLIED" 
Date: Fri, 19 Jan 2018 15:44:47 +
Subject:
To:


Hi.docx
Description: MS-Word 2007 document

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Fri, Jan 19, 2018 at 1:24 PM, Оля Тележная  wrote:
> 2018-01-18 17:23 GMT+03:00 Christian Couder :
>> On Thu, Jan 18, 2018 at 12:49 PM, Оля Тележная  
>> wrote:
>>> 2018-01-18 9:20 GMT+03:00 Оля Тележная :

 I think it's important to finish migrating process at first. I mean,
 now we are preparing and collecting everything in ref-filter, but we
 make resulting string and print still in cat-file. And I am not sure,
 but maybe it will not be possible to start using new atoms in cat-file
 while some part of logic still differs.
>>>
>>> I tried to make that part here:
>>> https://github.com/telezhnaya/git/commit/19a148614f1d4db1f8e628eb4e6d7c819d2da875
>>> I know that the code is disgusting and there is a memory leak :) I
>>> just try to reuse ref-filter logic, I will cleanup everything later.
>>> At first, I try to make it work.
>>> The problem is that I have segfault, and if I use gdb, I get:
>>>
>>> Program received signal SIGSEGV, Segmentation fault.
>>> 0x in ?? ()
>>
>> Make sure that you compile with debug options like -g3. For example I use:
>>
>> $ make -j 4 DEVELOPER=1 CFLAGS="-g3"
>
> Is it OK that I get different test results with simple make and with
> make with all that flags?
> Have a code: https://github.com/telezhnaya/git/commits/catfile
> I do:
>
> olya@ubuntu17-vm:~/git$ make install
> olya@ubuntu17-vm:~/git$ cd t
> olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh
>
> And I have 17 tests broken.
> Then, without any changes in code, I do:
>
> olya@ubuntu17-vm:~/git$ make -j 4 DEVELOPER=1 CFLAGS="-g3" install
> olya@ubuntu17-vm:~/git$ cd t
> olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh
>
> And there is 42 tests broken.
> And it's really hard to search for errors in such situation.

Some segfaults and other memory issues might happen or not happen
depending on how the code has been compiled. I think that if you fix
all the memory related issues, the behavior should be the same.

***BULK*** RE: Very Urgent

[GALLOY Anne]

Hello Dear,


Compliment of the season, I have an urgent Business Transaction for you kindly 
contact me through my Google email:  mrsjanezh...@gmail.com
for more details.

Happy New Year

Mrs Lin.

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

[Phillip Wood]

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?

Another possible optimization is that if the parent branches have only
reworded commits or some commits that have been squashed but no other
changes then their trees will be the same as in the original merge
commit and so could be reused without calling merge_recursive().

> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 21 -
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 567cfcbbe8b..a96255426e7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2085,7 +2085,7 @@ static int do_merge(struct commit *commit, const char 
> *arg, int arg_len,
>   struct commit *head_commit, *merge_commit, *i;
>   struct commit_list *common, *j, *reversed = NULL;
>   struct merge_options o;
> - int ret;
> + int can_fast_forward, ret;
>   static struct lock_file lock;
>  
>   for (merge_arg_len = 0; merge_arg_len < arg_len; merge_arg_len++)
> @@ -2151,6 +2151,14 @@ static int do_merge(struct commit *commit, const char 
> *arg, int arg_len,
>   return error(_("Cannot merge without a current revision"));
>   }
>  
> + /*
> +  * 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);
> +
>   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)) {
> + 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 2/8] sequencer: introduce the `merge` command

[Phillip Wood]

On 18/01/18 15:35, Johannes Schindelin wrote:
> 
> This patch is part of the effort to reimplement `--preserve-merges` with
> a substantially improved design, a design that has been developed in the
> Git for Windows project to maintain the dozens of Windows-specific patch
> series on top of upstream Git.
> 
> The previous patch implemented the `label`, `bud` and `reset` commands
> to label commits and to reset to a labeled commits. This patch adds the
> `merge` command, with the following syntax:
> 
>   merge   

I'm concerned that this will be confusing for users. All of the other
rebase commands replay the changes in the commit hash immediately
following the command name. This command instead uses the first commit
to specify the message which is different to both 'git merge' and the
existing rebase commands. I wonder if it would be clearer to have 'merge
-C   ...' instead so it's clear which argument specifies
the message and which the remote head to merge. It would also allow for
'merge -c   ...' in the future for rewording an existing
merge message and also avoid the slightly odd 'merge -  ...'. Where
it's creating new merges I'm not sure it's a good idea to encourage
people to only have oneline commit messages by making it harder to edit
them, perhaps it could take another argument to mean open the editor or
not, though as Jake said I guess it's not that common.

One thought that just struck me - if a merge or reset command specifies
an invalid label is it rescheduled so that it's still in the to-do list
when the user edits it after rebase stops?

In the future it might be nice if the label, reset and merge commands
were validated when the to-do list is parsed so that the user gets
immediate feedback if they try to create a label that is not a valid ref
name or that they have a typo in a name given to reset or merge rather
than the rebase stopping later.

> The  parameter in this instance is the *original* merge commit,
> whose author and message will be used for the to-be-created merge
> commit.
> 
> The  parameter refers to the (possibly rewritten) revision to
> merge. Let's see an example of a todo list:
> 
>   label onto
> 
>   # Branch abc
>   bud
>   pick deadbeef Hello, world!
>   label abc
> 
>   bud
>   pick cafecafe And now for something completely different
>   merge baaabaaa abc Merge the branch 'abc' into master
> 
> To support creating *new* merges, i.e. without copying the commit
> message from an existing commit, use the special value `-` as 
> parameter (in which case the text after the  parameter is used as
> commit message):
> 
>   merge - abc This will be the actual commit message of the merge
> 
> This comes in handy when splitting a branch into two or more branches.
> 
> Note: this patch only adds support for recursive merges, to keep things
> simple. Support for octopus merges will be added later in this patch
> series, support for merges using strategies other than the recursive
> merge is left for future contributions.
> 
> The design of the `merge` command as introduced by this patch only
> supports creating new merge commits with exactly two parents, i.e. it
> adds no support for octopus merges.
> 
> We will introduce support for octopus merges in a later commit.
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  git-rebase--interactive.sh |   1 +
>  sequencer.c| 146 
> +++--
>  2 files changed, 143 insertions(+), 4 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 3d2cd19d65a..5bf1ea3781f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -165,6 +165,7 @@ d, drop = remove commit
>  l, label = label current HEAD with a name
>  t, reset = reset HEAD to a label
>  b, bud = reset HEAD to the revision labeled 'onto'
> +m, merge = create a merge commit using a given commit's message
>  
>  These lines can be re-ordered; they are executed from top to bottom.
>  " | git stripspace --comment-lines >>"$todo"
> diff --git a/sequencer.c b/sequencer.c
> index 91cc55a002f..567cfcbbe8b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -779,6 +779,7 @@ enum todo_command {
>   TODO_LABEL,
>   TODO_RESET,
>   TODO_BUD,
> + TODO_MERGE,
>   /* commands that do nothing but are counted for reporting progress */
>   TODO_NOOP,
>   TODO_DROP,
> @@ -800,6 +801,7 @@ static struct {
>   { 'l', "label" },
>   { 't', "reset" },
>   { 'b', "bud" },
> + { 'm', "merge" },
>   { 0,   "noop" },
>   { 'd', "drop" },
>   { 0,   NULL }
> @@ -1304,14 +1306,20 @@ static int parse_insn_line(struct todo_item *item, 
> const char *bol, char *eol)
>   }
>  
>   end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> + item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> + item->arg_len = (int)(eol - item->arg);
> +
>   

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

[Phillip Wood]

From: Phillip Wood 

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 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 
---
 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..74b2eff71e886503d41b093953b9dd6ede29de3a
 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 $(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 &&
+   git commit &&
+   git rebase --continue &&
+   echo y>b &&
+   git 

[PATCH 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 
---
 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..79579ba118e2743c3463a8662368cb4008f02165
 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);
 }
 
+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 to write a tree"));
goto out;
@@ -1106,6 +1125,30 @@ static int try_to_commit(struct strbuf *msg, const char 
*author,
goto out;
}
 
+  

UTF-8-safe way for char-level-diff

[Danny Lin]

Git has a diff.wordRegex config that allows the user to specify a
regex that defines a word. Setting diff.wordRegex to "." works well
for a char-level diff for ASCII chars, but not for UTF-8 chars.

For example, if a file (encoded by UTF-8) with text "一人" is changed to
"丁人", "git diff --word-diff=color" gets "<80><81>人" (where
"<80>" is red and "<81>" is green) instead of desired "一丁人" (where "一"
is red and "丁" is green). This could be very annoying when diff-ing
files containing CJK chars.

Git diff.wordRegex seems to implement a very basic regex that doesn't
support matching char range by encoding such as "\x41" for "a". Is
there a way to make the char-level diff work correctly? If not, maybe
we should implement a way to allow it.

Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision

[Phillip Wood]

On 18/01/18 15:35, Johannes Schindelin wrote:
> 
> In the upcoming commits, we will teach the sequencer to recreate merges.
> This will be done in a very different way from the unfortunate design of
> `git rebase --preserve-merges` (which does not allow for reordering
> commits, or changing the branch topology).
> 
> The main idea is to introduce new todo list commands, to support
> labeling the current revision with a given name, resetting the current
> revision to a previous state, merging labeled revisions.

I think this would be a great improvement to rebase -i, thanks for
working on it.

> This idea was developed in Git for Windows' Git garden shears (that are
> used to maintain the "thicket of branches" on top of upstream Git), and
> this patch is part of the effort to make it available to a wider
> audience, as well as to make the entire process more robust (by
> implementing it in a safe and portable language rather than a Unix shell
> script).
> 
> This commit implements the commands to label, and to reset to, given
> revisions. The syntax is:
> 
>   label 
>   reset 

If I've understood the code below correctly then reset will clobber
untracked files, this is the opposite behaviour to what happens when
tries to checkout  at the start of a rebase - then it will fail if
untracked files would be overwritten.

> As a convenience shortcut, also to improve readability of the generated
> todo list, a third command is introduced: bud. It simply resets to the
> "onto" revision, i.e. the commit onto which we currently rebase.

I found the whole bud business bewildering at first, reading the other
replies it seems I wasn't the only one to be befuddled by it. Having
seen an example I can see what it's trying to do but I still think it
adds more confusion than value.

> Internally, the `label ` command creates the ref
> `refs/rewritten/`. This makes it possible to work with the labeled
> revisions interactively, or in a scripted fashion (e.g. via the todo
> list command `exec`).

If a user has two work trees and runs a rebase in each with the same
label name, they'll clobber each other. I'd suggest storing them under
refs/rewritten/ instead. If the user
tries to rebase a second worktree with the same detached HEAD as an
existing rebase then refuse to start.

> Signed-off-by: Johannes Schindelin 
> ---
>  git-rebase--interactive.sh |   3 +
>  sequencer.c| 181 
> -
>  2 files changed, 180 insertions(+), 4 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index d47bd29593a..3d2cd19d65a 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -162,6 +162,9 @@ s, squash = use commit, but meld into previous commit
>  f, fixup = like \"squash\", but discard this commit's log message
>  x, exec = run command (the rest of the line) using shell
>  d, drop = remove commit
> +l, label = label current HEAD with a name
> +t, reset = reset HEAD to a label
> +b, bud = reset HEAD to the revision labeled 'onto'
>  
>  These lines can be re-ordered; they are executed from top to bottom.
>  " | git stripspace --comment-lines >>"$todo"
> diff --git a/sequencer.c b/sequencer.c
> index 4d3f60594cb..91cc55a002f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -21,6 +21,8 @@
>  #include "log-tree.h"
>  #include "wt-status.h"
>  #include "hashmap.h"
> +#include "unpack-trees.h"
> +#include "worktree.h"
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> @@ -116,6 +118,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, 
> "rebase-merge/stopped-sha")
>  static GIT_PATH_FUNC(rebase_path_rewritten_list, 
> "rebase-merge/rewritten-list")
>  static GIT_PATH_FUNC(rebase_path_rewritten_pending,
>   "rebase-merge/rewritten-pending")
> +
> +/*
> + * The path of the file listing refs that need to be deleted after the rebase
> + * finishes. This is used by the `merge` command.
> + */
> +static GIT_PATH_FUNC(rebase_path_refs_to_delete, 
> "rebase-merge/refs-to-delete")
> +
>  /*
>   * The following files are written by git-rebase just after parsing the
>   * command-line (and are only consumed, not modified, by the sequencer).
> @@ -767,6 +776,9 @@ enum todo_command {
>   TODO_SQUASH,
>   /* commands that do something else than handling a single commit */
>   TODO_EXEC,
> + TODO_LABEL,
> + TODO_RESET,
> + TODO_BUD,
>   /* commands that do nothing but are counted for reporting progress */
>   TODO_NOOP,
>   TODO_DROP,
> @@ -785,6 +797,9 @@ static struct {
>   { 'f', "fixup" },
>   { 's', "squash" },
>   { 'x', "exec" },
> + { 'l', "label" },
> + { 't', "reset" },
> + { 'b', "bud" },
>   { 0,   "noop" },
>   { 'd', "drop" },
>   { 0,   NULL }
> @@ -1253,7 +1268,8 @@ static int parse_insn_line(struct todo_item *item, 
> const char *bol, char *eol)
>   if (skip_prefix(bol, 

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Оля Тележная]

2018-01-18 17:23 GMT+03:00 Christian Couder :
> On Thu, Jan 18, 2018 at 12:49 PM, Оля Тележная  
> wrote:
>> 2018-01-18 9:20 GMT+03:00 Оля Тележная :
>>>
>>> I think it's important to finish migrating process at first. I mean,
>>> now we are preparing and collecting everything in ref-filter, but we
>>> make resulting string and print still in cat-file. And I am not sure,
>>> but maybe it will not be possible to start using new atoms in cat-file
>>> while some part of logic still differs.
>>
>> I tried to make that part here:
>> https://github.com/telezhnaya/git/commit/19a148614f1d4db1f8e628eb4e6d7c819d2da875
>> I know that the code is disgusting and there is a memory leak :) I
>> just try to reuse ref-filter logic, I will cleanup everything later.
>> At first, I try to make it work.
>> The problem is that I have segfault, and if I use gdb, I get:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x in ?? ()
>
> Make sure that you compile with debug options like -g3. For example I use:
>
> $ make -j 4 DEVELOPER=1 CFLAGS="-g3"

Is it OK that I get different test results with simple make and with
make with all that flags?
Have a code: https://github.com/telezhnaya/git/commits/catfile
I do:

olya@ubuntu17-vm:~/git$ make install
olya@ubuntu17-vm:~/git$ cd t
olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh

And I have 17 tests broken.
Then, without any changes in code, I do:

olya@ubuntu17-vm:~/git$ make -j 4 DEVELOPER=1 CFLAGS="-g3" install
olya@ubuntu17-vm:~/git$ cd t
olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh

And there is 42 tests broken.
And it's really hard to search for errors in such situation.

>
>> I tried to google it, it's my first time when I get that strange
>> message, and unfortunately find nothing. So please explain me the
>> reason, why I can't find a place of segfault that way.
>
> I get the following:
>
> (gdb) run cat-file --batch < myarg.txt
> Starting program: /home/ubuntu/bin/git cat-file --batch < myarg.txt
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x556ea7cf in format_ref_array_item (info=0x7fffd460,
> format=0x7fffd6e0,
> final_buf=0x7fffd410) at ref-filter.c:2234
> 2234atomv->handler(atomv, );
> (gdb) bt
> #0  0x556ea7cf in format_ref_array_item (info=0x7fffd460,
> format=0x7fffd6e0, final_buf=0x7fffd410) at ref-filter.c:2234
> #1  0x556ea91c in show_ref_array_item (info=0x7fffd460,
> format=0x7fffd6e0)
> at ref-filter.c:2256
> #2  0x55577ef7 in batch_object_write (
> obj_name=0x55a66770 "5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689",
> opt=0x7fffd6e0, data=0x7fffd5e0) at builtin/cat-file.c:298

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

[Eric Sunshine]

On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
 wrote:
> [...]
> With this patch, the goodness of the Git garden shears comes to `git
> rebase -i` itself. Passing the `--recreate-merges` option will generate
> a todo list that can be understood readily, and where it is obvious
> how to reorder commits. New branches can be introduced by inserting
> `label` commands and calling `merge -  `. And once this
> mode has become stable and universally accepted, we can deprecate the
> design mistake that was `--preserve-merges`.
>
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> @@ -900,6 +900,7 @@ fi
>  if test t != "$preserve_merges"
>  then
> git rebase--helper --make-script ${keep_empty:+--keep-empty} \
> +   ${recreate_merges:+--recreate-merges} \

If the user specifies both --preserve-merges and --recreate-merges, it
looks like --preserve-merges will take precedence.

Should git-rebase.sh have a mutual-exclusion check and error out if
both are specified?

> $revisions ${restrict_revision+^$restrict_revision} >"$todo" 
> ||
> die "$(gettext "Could not generate todo list")"
> diff --git a/git-rebase.sh b/git-rebase.sh
> @@ -262,6 +264,10 @@ do
> +   --recreate-merges)
> +   recreate_merges=t
> +   test -z "$interactive_rebase" && interactive_rebase=implied
> +   ;;
> --preserve-merges)
> preserve_merges=t
> test -z "$interactive_rebase" && interactive_rebase=implied

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

[Eric Sunshine]

On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
 wrote:
> 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).
> [...]
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -2785,6 +2787,335 @@ void append_signoff(struct strbuf *msgbuf, int 
> ignore_footer, unsigned flag)
> +static const char *label_oid(struct object_id *oid, const char *label,
> +struct label_state *state)
> +{
> +   [...]
> +   } else if (((len = strlen(label)) == GIT_SHA1_RAWSZ &&
> +   !get_oid_hex(label, )) ||
> +  hashmap_get_from_hash(>labels,
> +strihash(label), label)) {
> +   /*
> +* If the label already exists, or if the label is a valid 
> full
> +* OID, we append a dash and a number to make it unique.
> +*/
> +   [...]
> +   for (i = 2; ; i++) {

Why '2'? Is there some non-obvious significance to this value?

> +   strbuf_setlen(buf, len);
> +   strbuf_addf(buf, "-%d", i);
> +   if (!hashmap_get_from_hash(>labels,
> +  strihash(buf->buf),
> +  buf->buf))
> +   break;
> +   }
> +
> +static int make_script_with_merges(struct pretty_print_context *pp,
> +  struct rev_info *revs, FILE *out,
> +  unsigned flags)
> +{
> +   [...]
> +   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?

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Eric Sunshine]

On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
 wrote:
> [...]
> Note: this patch only adds support for recursive merges, to keep things
> simple. Support for octopus merges will be added later in this patch
> series, support for merges using strategies other than the recursive
> merge is left for future contributions.

The above paragraph...

> The design of the `merge` command as introduced by this patch only
> supports creating new merge commits with exactly two parents, i.e. it
> adds no support for octopus merges.
>
> We will introduce support for octopus merges in a later commit.

and these two sentences say the same thing. I suppose one or the other
was meant to be dropped(?).

> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -2069,6 +2077,132 @@ static int do_reset(const char *name, int len)
> +static int do_merge(struct commit *commit, const char *arg, int arg_len,
> +   struct replay_opts *opts)
> +{
> +   [...]
> +   if (write_message(body, len, git_path_merge_msg(), 0) < 0) {
> +   error_errno(_("Could not write '%s'"),

s/Could/could/

> +   if (write_message(p, len, git_path_merge_msg(), 0) < 0) {
> +   error_errno(_("Could not write '%s'"),

Ditto.

> +   if (!head_commit) {
> +   rollback_lock_file();
> +   return error(_("Cannot merge without a current revision"));

s/Cannot/cannot/

Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision

[Eric Sunshine]

On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
 wrote:
> [...]
> This commit implements the commands to label, and to reset to, given
> revisions. The syntax is:
>
> label 
> reset 
> [...]
>
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -1919,6 +1936,139 @@ static int do_exec(const char *command_line)
> +static int safe_append(const char *filename, const char *fmt, ...)
> +{
> +   [...]
> +   if (commit_lock_file() < 0) {
> +   rollback_lock_file();
> +   return error(_("failed to finalize '%s'."), filename);

s/\.//

> +   }
> +
> +   return 0;
> +}
> +
> +static int do_reset(const char *name, int len)
> +{
> +   [...]
> +   if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0)
> +   return -1;
> +
> +   for (i = 0; i < len; i++)
> +   if (isspace(name[i]))
> +   len = i;

What is the purpose of this loop? I could imagine that it's trying to
strip all whitespace from the end of 'name', however, to do that it
would iterate backward, not forward. (Or perhaps it's trying to
truncate at the first space, but then it would need to invert the
condition or use 'break'.) Am I missing something obvious?

> +   read_cache_unmerged();
> +   if (!fill_tree_descriptor(, )) {
> +   error(_("Failed to find tree of %s."), oid_to_hex());

s/Failed/failed/
s/\.//

> +   rollback_lock_file();
> +   free((void *)desc.buffer);
> +   strbuf_release(_name);
> +   return -1;
> +   }

Re: git 2.16.0 segfaults on clone of specific repo

[Eric Sunshine]

On Fri, Jan 19, 2018 at 3:22 AM, Duy Nguyen  wrote:
> On Fri, Jan 19, 2018 at 2:40 PM, Eric Sunshine  
> wrote:
>> On Fri, Jan 19, 2018 at 12:31:58PM +0700, Duy Nguyen wrote:
>>> On Linux, after I hacked all over the place to force ce_match_stat()
>>> to eventually call index_fd() which in turns must use one of the
>>> hashing function, I got a crash.
>>
>> Nice detective work.
>
> And not stepping back to think for a bit. I realized now that if I
> just mounted a vfat filesystem, I could have verified it much quicker.
> It does crash on linux with similar stack trace.
> [...]
>> This particular manifestation is caught by the
>> following test which fails without brian's patch on MacOS (and
>> presumably Windows) and succeeds with it. On Linux and BSD, it will of
>> course succeed always, so I'm not sure how much practical value it
>> has.
>
> There's a travis job running "on windows" (I don't what it really
> means) so it might help actually. This vim-colorschemes repository has
> shown up in git mailing list before, I think. We probably should just
> add it as part of our regression tests ;-)

And, Dscho does run the test suite on Windows regularly, so perhaps
this test does have some practical value.

Re: git 2.16.0 segfaults on clone of specific repo

[Duy Nguyen]

On Fri, Jan 19, 2018 at 2:40 PM, Eric Sunshine  wrote:
> On Fri, Jan 19, 2018 at 12:31:58PM +0700, Duy Nguyen wrote:
>> On Fri, Jan 19, 2018 at 10:40 AM, brian m. carlson
>> > I'm still extremely puzzled as to why this doesn't fail on Linux.  If
>> > it's failing in this case, it should very, very clearly fail all the
>> > time we access an empty blob or an empty tree.[...]
>>
>> I think it's file system related, case-insensitive one in particular.
>>
>> The call trace posted at the beginning of this thread should never
>> trigger for an initial clone. You only check if an _existing_ entry
>> matches what you are about to checkout when you switch trees. For this
>> to happen at clone time, I suppose you have to checkout entry "A",
>> then re-checkout "A" again. Which can only happen on case-insensitive
>> file systems and a case-sensitive repo where the second "A" might
>> actually be "a".
>> [...]
>> On Linux, after I hacked all over the place to force ce_match_stat()
>> to eventually call index_fd() which in turns must use one of the
>> hashing function, I got a crash.
>
> Nice detective work.

And not stepping back to think for a bit. I realized now that if I
just mounted a vfat filesystem, I could have verified it much quicker.
It does crash on linux with similar stack trace.

(gdb) bt
#0  0x0055809f in is_empty_blob_sha1 (sha1=0x8fa6d4 "\236") at
cache.h:1055
#1  0x00558acd in ce_match_stat_basic (ce=0x8fa690,
st=0x7fffcd20) at read-cache.c:272
#2  0x00558c78 in ie_match_stat (istate=0x8e9fc0 ,
ce=0x8fa690, st=0x7fffcd20, options=5) at read-cache.c:342
#3  0x00501e01 in checkout_entry (ce=0x8fa690,
state=0x7fffcea0, topath=0x0) at entry.c:424
#4  0x005c8ea0 in check_updates (o=0x7fffd060) at unpack-trees.c:383
#5  0x005cb2bb in unpack_trees (len=1, t=0x7fffd010,
o=0x7fffd060) at unpack-trees.c:1382
#6  0x004214ca in checkout (submodule_progress=1) at builtin/clone.c:750
#7  0x004229ce in cmd_clone (argc=1, argv=0x7fffd840,
prefix=0x0) at builtin/clone.c:1191
#8  0x00405846 in run_builtin (p=0x89a908 ,
argc=2, argv=0x7fffd840) at git.c:346
#9  0x00405af7 in handle_builtin (argc=2, argv=0x7fffd840)
at git.c:554
#10 0x00405c8f in run_argv (argcp=0x7fffd6fc,
argv=0x7fffd6f0) at git.c:606
#11 0x00405e31 in cmd_main (argc=2, argv=0x7fffd840) at git.c:683
#12 0x004a3231 in main (argc=3, argv=0x7fffd838) at common-main.c:43

> This particular manifestation is caught by the
> following test which fails without brian's patch on MacOS (and
> presumably Windows) and succeeds with it. On Linux and BSD, it will of
> course succeed always, so I'm not sure how much practical value it
> has.

There's a travis job running "on windows" (I don't what it really
means) so it might help actually. This vim-colorschemes repository has
shown up in git mailing list before, I think. We probably should just
add it as part of our regression tests ;-)
-- 
Duy