Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 11.07.2017 um 02:05 schrieb brian m. carlson:

On Mon, Jul 10, 2017 at 09:57:40PM +0200, Johannes Sixt wrote:

It's a pity, though, that you do not suggest something even more useful,
such as C++14.


I have tried compiling Git with a C++ compiler, so that I could test
whether that was a viable alternative for MSVC in case its C++ mode
supported features its C mode did not.  Let's just say that the
compilation aborted very quickly and I gave up after a few minutes.


It's 3 cleanup patches and one hacky patch with this size:

 80 files changed, 899 insertions(+), 807 deletions(-)

to compile with C++. It passed the test suite last time I tried. Getting 
rid of the remaining 1000+ -fpermissive warnings is a different matter, 
though.


I can revive the patches if there is interest.

-- Hannes

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Mike Hommey]

On Mon, Jul 10, 2017 at 10:57:57AM -0400, Ben Peart wrote:
> Correct.  MSVC also supports designated initializers but does not fully
> support C99.

Precision: *recent versions* of MSVC support designated initializer.
2013 introduced them, but there were bugs until 2015, see e.g.
https://stackoverflow.com/questions/24090739/possible-compiler-bug-in-msvc12-vs2013-with-designated-initializer

Mike

Re: Flurries of 'git reflog expire'

[Andreas Krey]

On Thu, 06 Jul 2017 10:01:05 +, Bryan Turner wrote:

> Do you know what version of Bitbucket Server is in use?

We're on the newest 4.x.

...
> - Run "git config gc.auto 0" in

Going that route.

...
> I also want to add that Bitbucket Server 5.x includes totally
> rewritten GC handling. 5.0.x automatically disables auto GC in all
> repositories and manages it explicitly, and 5.1.x fully removes use of
> "git gc" in favor of running relevant plumbing commands directly.

That's the part that irks me. This shouldn't be necessary - git itself
should make sure auto GC isn't run in parallel. Now I probably can't
evaluate whether a git upgrade would fix this, but given that you
are going the do-gc-ourselves route I suppose it wouldn't.

...
> Upgrading to 5.x can be a bit of an undertaking, since the major
> version brings API changes,

The upgrade is on my todo list, but there are plugins that don't
appear to be ready for 5.0, notable the jenkins one.

Andreas

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

Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize

[Jeff King]

On Mon, Jul 10, 2017 at 09:02:24PM +0100, Ramsay Jones wrote:

> After a quick look at the ./t-basic.sh test, I managed to get
> the test to complete (with 15 tests failing), with the following
> patch applied:
> 
> -- >8 --
> diff --git a/Makefile b/Makefile
> index 3c341b2a6..8e6433738 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1016,7 +1016,7 @@ ifdef SANITIZE
>  BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
>  BASIC_CFLAGS += -fno-omit-frame-pointer
>  ifeq ($(SANITIZE),undefined)
> -BASIC_CFLAGS += -DNO_UNALIGNED_LOADS
> +BASIC_CFLAGS += -DNO_UNALIGNED_LOADS -DSHA1DC_FORCE_ALIGNED_ACCESS
>  endif
>  endif

Thanks, I forgot to mention SHA1DC. When I had originally tested with
"undefined", it was before we had SHA1DC. I hacked around it earlier
today by just using OPENSSL_SHA1. ;)

I agree if we can ask it to avoid unaligned access that is even better.

> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
> index 25eded139..3baddc636 100644
> --- a/sha1dc/sha1.c
> +++ b/sha1dc/sha1.c
> @@ -118,6 +118,10 @@
>  #define SHA1DC_ALLOW_UNALIGNED_ACCESS
>  #endif /*UNALIGNMENT DETECTION*/
>  
> +#if defined(SHA1DC_ALLOW_UNALIGNED_ACCESS) && 
> defined(SHA1DC_FORCE_ALIGNED_ACCESS)
> +#undef SHA1DC_ALLOW_UNALIGNED_ACCESS
> +#endif

I think our current strategy is to avoid touching sha1.c as much as
possible. I think we'd prefer a patch to the upstream project to support
FORCE_ALIGNED_ACCESS (unfortunately I do not see a way to tweak it using
only external defines.

-Peff

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Jeff King]

On Mon, Jul 10, 2017 at 11:11:35PM +0200, Johannes Sixt wrote:

> > I am not sure what negative impact you think the macro-ness would
> > have to the validity of the result from this test balloon.  An old
> > compiler that does not understand designated initializer syntax
> > would fail to compile both the same way, no?
> > 
> > struct strbuf buf0 = STRBUF_INIT;
> > struct strbuf buf1 = { .alloc = 0, .len = 0, .buf = strbuf_slopbuf };
> 
> I said it is uninteresting, not that there is a negative impact. There is
> simply nothing gained for strbuf users: They would use STRBUF_INIT before
> and after the change and would not benefit from designated initializers.
> 
> This change may serve well as a test balloon, but not as an example of the
> sort of changes that we would want to see later (of the kind "change
> FOO_INIT macro to use designated initializers"; they are just code churn).

But that is exactly the point. First the test balloon, wait a release or
two, and then make real useful changes. The purpose of the test balloon
is to gather data with minimal impact. To be useful, the changes would
be pervasive, and that would not have minimal impact.

-Peff

Re: Weirdness with git change detection

[Torsten Bögershausen]

On 11/07/17 01:45, Peter Eckersley wrote:

I have a local git repo that's in some weird state where changes
appear to be detected by "git diff" and prevent operations like "git
checkout" from switching branches, but those changes are not removed
by a "git reset --hard" or "git stash".

Here's an example of the behaviour, with "git reset --hard" failing to
clear a diff in the index:

https://paste.debian.net/975811/

Happy to collect additional debugging information if it's useful.


If possible, we need to clone the repo and debug ourselfs - in other
words, the problem must be reproducible for others.

It the repo public ?
Which OS, Git version are you using ?

bug: HEAD vs. head on case-insensitive filesystems

[Kenneth Hsu]

I'm not sure what the general consensus is regarding the use of "head"
vs. "HEAD" on case insensitive filesystems, but it appears that some
confusing behavior (bug?) may have arisen.

To summarize, "HEAD" and "head" may resolve to different revisions when
in a worktree.  The following example was generated using git version
2.13.1 for Mac (HFS+):

$ git --version
git version 2.13.1
$ git init
Initialized empty Git repository in /Users/ken/Desktop/test/.git/

$ echo "Hello" > hello.txt
$ git add . && git commit -qm "Add hello."

$ echo "Bye" > bye.txt
$ git add . && git commit -qm "Add bye."

Note that at this point, both HEAD and head (correctly) resolve to the
same revision:

$ git rev-parse HEAD
4a71a947fb683698f80f543f9cd27acd066e2659

$ git rev-parse head
4a71a947fb683698f80f543f9cd27acd066e2659

However, if we create (and cd into) a worktree based on "master~", "HEAD" and
"head" resolve to _different_ revisions:

$ git worktree add -b feature/branch ../branch master~
Preparing ../branch (identifier branch)
HEAD is now at f752545 Add hello.
$ cd ../branch/

$ git rev-parse HEAD
f7525451640f6f5e8842cc00b6639c80558dd6c2

$ git rev-parse head
4a71a947fb683698f80f543f9cd27acd066e2659

$ git rev-parse master
4a71a947fb683698f80f543f9cd27acd066e2659

$ git rev-parse master~
f7525451640f6f5e8842cc00b6639c80558dd6c2

$ git rev-parse feature/branch
f7525451640f6f5e8842cc00b6639c80558dd6c2

Note that "HEAD" resolves to the same revision as "master~" and
"feature/branch" (which seems correct since that is what the worktree
was based on), while "head" resolves to the same revision as "master".

This appears to affect other case-insensitive filesystems (Windows) as
well.  See the following bug report:

https://github.com/git-for-windows/git/issues/1225

I'm not sure if the behavior is well-defined when using "head", but the
above example may illustrate a case where users should not assume that
they resolve to the same thing.

Thanks.

Re: [RFC/WIP PATCH] object store classification

[Stefan Beller]

On Fri, Jul 7, 2017 at 9:50 AM, Junio C Hamano  wrote:
> Ben Peart  writes:
>
>> For more API/state design purity, I wonder if there should be an
>> object_store structure that is passed to each of the object store APIs
>> instead of passing the repository object. The repository object could
>> then contain an instance of the object_store structure.
>>
>> That said, I haven't take a close look at all the code in object.c to
>> see if all the data needed can be cleanly abstracted into an
>> object_store structure.
>
> My gut feeling was it is just the large hashtable that keeps track of
> objects we have seen, but the object replacement/grafts and other
> things may also want to become per-repository.

This is similar to the_index which is referenced by the_repository.
But as we do not have anything like the_object_store already, we are
free to design it, as the required work that needs to be put in is the
same.

With the object replacements/grafts coming up as well as alternates,
we definitely want that to be per repository, the question is if we rather
want

  the_repository -> many object_stores (one for each, alternate, grafts,
  and the usual place at $GIT_DIR/objects
  where the object_store is a hashmap, maybe an additional describing
  string or path.

or

  the_repository -> the_object_store
  but the object store is a complex beast having different hash tables
  for the different alternates.

or

  the_repository -> the_object_store_hash_map
  which is this patch that would try to put any object related to this
  repository into the same hashmap and the hashmap is not special
  for each of the different object locations.


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

Sounds reasonable to expect.

Thanks,
Stefan

[PATCH] RFC: A new type of symbolic refs

[Stefan Beller]

A symbolic ref can currently only point at (another symbolic) ref.
However are use cases for symbolic refs to point at other things
representing a commit-ish, such as gitlink entries in other
repositories.  In this use case we can use such a symbolic link
to strengthen the relationship between a submodule and a superproject.
Examples:

1) It makes it easier to explain when we recurse into submodules and
   to which sha1 the submodule is updated.

   Currently a submodule (or any repo) is either on a branch (i.e.
   HEAD is a symbolic ref) or is in 'detached HEAD' state (HEAD is
   a direct ref).
   For submodules it is wrong to be on its own branch if it wants to be
   controlled by the superproject as being on its own branch signals that
   the submodule is independent and can move the HEAD freely.
   Being in 'detached HEAD' state is the alternative to that and was
   chosen when git-submodule was implemented, but it is equally wrong;
   the lesser of two evils.

   Semantically the correct way to state a submodule is part of the
   superproject is by pointing its HEAD to the superproject.

   We do have "reset/checkout --recurse-submodules" now, but it is
   hard to explain what actually happens there (Which submodules are
   updated? All of them! -- But I want a subset only!)

   With this new mode of symbolic refs, any submodule that tracks the
   superproject, would 'automatically follow' the superproject as the
   submodules HEAD moves when the superproject changes.

2) "git -C submodule commit" would behave the same as it would on branch
   nowadays: It would make the commit in the submodule and then change
   the target of the symbolic ref which would be the index of the
   superproject! That implies that you do not need to 'git add' the
   submodule to the superproject, but have it done automatically.

Signed-off-by: Stefan Beller 
---
 cache.h  |  2 ++
 refs/files-backend.c | 10 ++
 submodule.c  | 40 
 3 files changed, 52 insertions(+)

diff --git a/cache.h b/cache.h
index 71fe092644..4f79d23202 100644
--- a/cache.h
+++ b/cache.h
@@ -2029,4 +2029,6 @@ void sleep_millisec(int millisec);
  */
 void safe_create_dir(const char *dir, int share);
 
+extern int read_external_symref(struct strbuf *from, struct strbuf *out);
+
 #endif /* CACHE_H */
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0404f2c233..f56f7b87ce 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -713,6 +713,16 @@ static int files_read_raw_ref(struct ref_store *ref_store,
goto out;
}
 
+   if (starts_with(buf, "repo:")) {
+   if (read_external_symref(_contents, referent)) {
+   *type |= REF_ISBROKEN;
+   ret = -1;
+   goto out;
+   }
+   *type |= REF_ISSYMREF;
+   ret = 0;
+   }
+
/*
 * Please note that FETCH_HEAD has additional
 * data after the sha.
diff --git a/submodule.c b/submodule.c
index da2b484879..7297f90485 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2037,3 +2037,43 @@ int submodule_to_gitdir(struct strbuf *buf, const char 
*submodule)
 cleanup:
return ret;
 }
+
+int read_external_symref(struct strbuf *from, struct strbuf *out)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+   const char *repo, *gitlink;
+   int hint, code;
+   struct strbuf **split = strbuf_split(from, 0);
+   struct strbuf cmd_out = STRBUF_INIT;
+
+   if (!split[0] || !split[1])
+   return -1;
+
+   repo = split[0]->buf + 5; /* skip 'repo:' */
+   gitlink = split[1]->buf;
+
+   argv_array_pushl(,
+   "ignored-first-arg",
+   "-C", repo,
+   "ls-tree", "-z", "HEAD", "--", gitlink, NULL);
+
+   /*
+* 17 accounts for '16 commit ',
+* the \t before path and trailing \0.
+*/
+   hint = 17 + GIT_SHA1_HEXSZ + split[1]->len;
+   code = capture_command(, _out, hint);
+
+   strbuf_release(split[0]);
+   strbuf_release(split[1]);
+
+   if (!code) {
+   strbuf_reset(out);
+   strbuf_add(out, cmd_out.buf + strlen("16 commit "),
+  GIT_SHA1_HEXSZ);
+   } else
+   return -1;
+
+   return 0;
+}
+
-- 
2.13.2.695.g117ddefdb4

Re: pre-rebase hook: capture documentation in a <

[brian m. carlson]

On Mon, Jul 10, 2017 at 04:35:25PM -0700, Stefan Beller wrote:
> Junio wrote in "What's-cooking":
> 
> > ... I do not know how well they are tested
> > in the field by people using 'master' in their everyday workflow.
> > Ideally, our release process wants to see more people using 'next'
> > in their everyday workflow to keep 'master' more stable than any
> > tagged release, but I do not have a good idea on how to encourage
> > it more than we currently do.
> 
> Our internal release of git @ Google is debian experimental,
> which is basically the 'next' branch + this patch + another patch.
> 
> AFAICT It is a resend of
> https://public-inbox.org/git/20120308122105.GA1562@burratino/
> 
> As Jonathan is a Debian Developer, it is easy for us to base
> our internal version onto debian experimental, but long term we may
> want to base our internal version on the original next. :)
> To do so, upstream this one last meaningful patch.
> 
> The 'another patch' from above is changing and hardcoding
> the version number, which we do not want to upstream.

Thanks for sending this in.  I had wanted to do so myself so I could
easily automate building Git packages based on the Debian packaging, but
I never got around to it.
-- 
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] strbuf: use designated initializers in STRBUF_INIT

[brian m. carlson]

On Mon, Jul 10, 2017 at 05:07:43PM -0700, Stefan Beller wrote:
> On Mon, Jul 10, 2017 at 5:05 PM, brian m. carlson
>  wrote:
> > On Mon, Jul 10, 2017 at 09:57:40PM +0200, Johannes Sixt wrote:
> >> It's a pity, though, that you do not suggest something even more useful,
> >> such as C++14.
> >
> > I have tried compiling Git with a C++ compiler, so that I could test
> > whether that was a viable alternative for MSVC in case its C++ mode
> > supported features its C mode did not.  Let's just say that the
> > compilation aborted very quickly and I gave up after a few minutes.
> 
> ... because we use reserved C++ keywords such as 'new' as variable names?

Yes, that was part of it ("template" stuck out to me).  I don't remember
all the issues, but they seemed quite numerous.  I'm sure it can be
done, though, if it's valuable to someone.
-- 
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] strbuf: use designated initializers in STRBUF_INIT

[Stefan Beller]

On Mon, Jul 10, 2017 at 5:05 PM, brian m. carlson
 wrote:
> On Mon, Jul 10, 2017 at 09:57:40PM +0200, Johannes Sixt wrote:
>> It's a pity, though, that you do not suggest something even more useful,
>> such as C++14.
>
> I have tried compiling Git with a C++ compiler, so that I could test
> whether that was a viable alternative for MSVC in case its C++ mode
> supported features its C mode did not.  Let's just say that the
> compilation aborted very quickly and I gave up after a few minutes.

... because we use reserved C++ keywords such as 'new' as variable names?

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[brian m. carlson]

On Mon, Jul 10, 2017 at 09:57:40PM +0200, Johannes Sixt wrote:
> It's a pity, though, that you do not suggest something even more useful,
> such as C++14.

I have tried compiling Git with a C++ compiler, so that I could test
whether that was a viable alternative for MSVC in case its C++ mode
supported features its C mode did not.  Let's just say that the
compilation aborted very quickly and I gave up after a few minutes.
-- 
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: Bug - Dirty submodule differences between OSX/Ubuntu

[Stefan Beller]

On Mon, Jul 10, 2017 at 4:53 PM, brian m. carlson
 wrote:
> On Sun, Jul 09, 2017 at 01:42:47PM -0700, Steve Kallestad wrote:
>> change into the submodule directory and run status
>> cd python-mode.el
>> git status
>>
>> On ubuntu:
>> On branch master
>> Your branch is up-to-date with 'origin/master'.
>> nothing to commit, working directory clean
>>
>> On OSX:
>> On branch master
>> Your branch is up-to-date with 'origin/master'.
>> Changes not staged for commit:
>>   (use "git add/rm ..." to update what will be committed)
>>   (use "git checkout -- ..." to discard changes in working directory)
>>
>> deleted:EXTENSIONS
>>
>> no changes added to commit (use "git add" and/or "git commit -a")
>
> This is an artifact of using a case-insensitive file system.  There is a
> directory called "extensions" and so git has not checked out the file
> "EXTENSIONS", as there's already a file system object.  It therefore
> sees it as deleted, since git tracks only files (and not really
> directories, but trees of files).
>
> This repository is always going to show as modified on a
> case-insensitive file system.  You can either ask the maintainers to
> change it, or reformat your disk with a case-sensitive file system.

While this is certainly not the answer Steve hoped for, we should
see if there are any implications by the user of submodules, i.e.
is this behavior reproducable without submodules? (In case it is not,
do we want to have the same checks in place for gitlinks as for
directories?)

Re: Bug - Dirty submodule differences between OSX/Ubuntu

[brian m. carlson]

On Sun, Jul 09, 2017 at 01:42:47PM -0700, Steve Kallestad wrote:
> change into the submodule directory and run status
> cd python-mode.el
> git status
> 
> On ubuntu:
> On branch master
> Your branch is up-to-date with 'origin/master'.
> nothing to commit, working directory clean
> 
> On OSX:
> On branch master
> Your branch is up-to-date with 'origin/master'.
> Changes not staged for commit:
>   (use "git add/rm ..." to update what will be committed)
>   (use "git checkout -- ..." to discard changes in working directory)
> 
> deleted:EXTENSIONS
> 
> no changes added to commit (use "git add" and/or "git commit -a")

This is an artifact of using a case-insensitive file system.  There is a
directory called "extensions" and so git has not checked out the file
"EXTENSIONS", as there's already a file system object.  It therefore
sees it as deleted, since git tracks only files (and not really
directories, but trees of files).

This repository is always going to show as modified on a
case-insensitive file system.  You can either ask the maintainers to
change it, or reformat your disk with a case-sensitive file system.
-- 
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

Weirdness with git change detection

[Peter Eckersley]

I have a local git repo that's in some weird state where changes
appear to be detected by "git diff" and prevent operations like "git
checkout" from switching branches, but those changes are not removed
by a "git reset --hard" or "git stash".

Here's an example of the behaviour, with "git reset --hard" failing to
clear a diff in the index:

https://paste.debian.net/975811/

Happy to collect additional debugging information if it's useful.
-- 
Peter

Re: [GSoC][PATCH 1/8] submodule--helper: introduce get_submodule_displaypath()

[Brandon Williams]

On 07/10, Stefan Beller wrote:
> On Mon, Jul 10, 2017 at 4:32 PM, Brandon Williams  wrote:
> >>   if (!is_submodule_active(the_repository, path)) {
> >> - strbuf_reset();
> >
> > Is this line removal intended?  It doesn't look related to the rest of
> > this patch.
> 
> It is, as  is re-used and has to be cleared first.
> With the code above removed,  is unmodified since
> struct strbuf sb = STRBUF_INIT; hence the removal is ok here.
> It is related, but only when looking at the entirety of the code. :-/

Ah I see. Thanks!

-- 
Brandon Williams

Re: [GSoC][PATCH 5/8] submodule: port submodule subcommand 'sync' from shell to C

[Brandon Williams]

On 07/11, Prathamesh Chavan wrote:
> Port the submodule subcommand 'sync' from shell to C using the same
> mechanism as that used for porting submodule subcommand 'status'.
> Hence, here the function cmd_sync() is ported from shell to C.
> This is done by introducing three functions: module_sync(),
> sync_submodule() and print_default_remote().
> 
> The function print_default_remote() is introduced for getting
> the default remote as stdout.
> 
> Mentored-by: Christian Couder 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
>  builtin/submodule--helper.c | 181 
> +++-
>  git-submodule.sh|  56 +-
>  2 files changed, 181 insertions(+), 56 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 980b8ed27..4e04b93f3 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -44,6 +44,20 @@ static char *get_default_remote(void)
>   return ret;
>  }
>  
> +static int print_default_remote(int argc, const char **argv, const char 
> *prefix)
> +{
> + const char *remote;
> +
> + if (argc != 1)
> + die(_("submodule--helper print-default-remote takes no 
> arguments"));
> +
> + remote = get_default_remote();
> + if (remote)
> + puts(remote);
> +
> + return 0;
> +}
> +
>  static int starts_with_dot_slash(const char *str)
>  {
>   return str[0] == '.' && is_dir_sep(str[1]);
> @@ -379,6 +393,25 @@ static void module_list_active(struct module_list *list)
>   *list = active_modules;
>  }
>  
> +static char *get_up_path(const char *path)
> +{
> + int i;
> + struct strbuf sb = STRBUF_INIT;
> +
> + for (i = count_slashes(path); i; i--)
> + strbuf_addstr(, "../");
> +
> + /*
> +  * Check if 'path' ends with slash or not
> +  * for having the same output for dir/sub_dir
> +  * and dir/sub_dir/
> +  */
> + if (!is_dir_sep(path[strlen(path) - 1]))
> + strbuf_addstr(, "../");
> +
> + return strbuf_detach(, NULL);
> +}
> +
>  static int module_list(int argc, const char **argv, const char *prefix)
>  {
>   int i;
> @@ -478,16 +511,18 @@ static void init_submodule(const struct cache_entry 
> *list_item, void *cb_data)
>   char *remote = get_default_remote();
>   struct strbuf remotesb = STRBUF_INIT;
>   strbuf_addf(, "remote.%s.url", remote);
> - free(remote);
>  
>   if (git_config_get_string(remotesb.buf, )) {
>   warning(_("could not lookup configuration '%s'. 
> Assuming this repository is its own authoritative upstream."), remotesb.buf);
>   remoteurl = xgetcwd();
>   }
>   relurl = relative_url(remoteurl, url, NULL);
> +
> + free(remote);
>   strbuf_release();
>   free(remoteurl);
>   free(url);
> +
>   url = relurl;

The changes in this function seem unintended.

>   }
>  
> @@ -732,6 +767,148 @@ static int module_name(int argc, const char **argv, 
> const char *prefix)
>   return 0;
>  }
>  
> +struct sync_cb {
> + const char *prefix;
> + unsigned int quiet: 1;
> + unsigned int recursive: 1;
> +};
> +#define SYNC_CB_INIT { NULL, 0, 0 }
> +
> +static void sync_submodule(const struct cache_entry *list_item, void 
> *cb_data)
> +{
> + struct sync_cb *info = cb_data;
> + const struct submodule *sub;
> + char *sub_key, *remote_key;
> + char *sub_origin_url, *super_config_url, *displaypath;
> + struct strbuf sb = STRBUF_INIT;
> + struct child_process cp = CHILD_PROCESS_INIT;
> +
> + if (!is_submodule_active(the_repository, list_item->name))
> + return;
> +
> + sub = submodule_from_path(null_sha1, list_item->name);
> +
> + if (!sub || !sub->url)
> + die(_("no url found for submodule path '%s' in .gitmodules"),
> +   list_item->name);
> +
> + if (starts_with_dot_dot_slash(sub->url) || 
> starts_with_dot_slash(sub->url)) {
> + char *remote_url, *up_path;
> + char *remote = get_default_remote();
> + char *remote_key = xstrfmt("remote.%s.url", remote);
> + free(remote);
> +
> + if (git_config_get_string(remote_key, _url))
> + remote_url = xgetcwd();
> + up_path = get_up_path(list_item->name);
> + sub_origin_url = relative_url(remote_url, sub->url, up_path);
> + super_config_url = relative_url(remote_url, sub->url, NULL);
> + free(remote_key);
> + free(up_path);
> + free(remote_url);
> + } else {
> + sub_origin_url = xstrdup(sub->url);
> 

Re: [GSoC][PATCH 1/8] submodule--helper: introduce get_submodule_displaypath()

[Stefan Beller]

On Mon, Jul 10, 2017 at 4:32 PM, Brandon Williams  wrote:
>>   if (!is_submodule_active(the_repository, path)) {
>> - strbuf_reset();
>
> Is this line removal intended?  It doesn't look related to the rest of
> this patch.

It is, as  is re-used and has to be cleared first.
With the code above removed,  is unmodified since
struct strbuf sb = STRBUF_INIT; hence the removal is ok here.
It is related, but only when looking at the entirety of the code. :-/

Re: [GSoC][PATCH 4/8] submodule: port submodule subcommand 'status' from shell to C

[Brandon Williams]

On 07/11, Prathamesh Chavan wrote:
> This aims to make git-submodule 'status' a built-in. Hence, the function
> cmd_status() is ported from shell to C. This is done by introducing
> three functions: module_status(), submodule_status() and print_status().
> 
> The function module_status() acts as the front-end of the subcommand.
> It parses subcommand's options and then calls the function
> module_list_compute() for computing the list of submodules. Then
> this functions calls for_each_submodule_list() looping through the
> list obtained.
> 
> Then for_each_submodule_list() calls submodule_status() for each of the
> submodule in its list. The function submodule_status() is responsible
> for generating the status each submodule it is called for, and
> then calls print_status().
> 
> Finally, the function print_status() handles the printing of submodule's
> status.
> 
> Mentored-by: Christian Couder 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
>  builtin/submodule--helper.c | 154 
> 
>  git-submodule.sh|  49 +-
>  2 files changed, 155 insertions(+), 48 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 80f744407..980b8ed27 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -560,6 +560,159 @@ static int module_init(int argc, const char **argv, 
> const char *prefix)
>   return 0;
>  }
>  
> +struct status_cb {
> + const char *prefix;
> + unsigned int quiet: 1;
> + unsigned int recursive: 1;
> + unsigned int cached: 1;
> +};
> +#define STATUS_CB_INIT { NULL, 0, 0, 0 }
> +
> +static void print_status(struct status_cb *info, char state, const char 
> *path,
> +  char *sub_sha1, char *displaypath)
> +{
> + if (info->quiet)
> + return;
> +
> + printf("%c%s %s", state, sub_sha1, displaypath);
> +
> + if (state == ' ' || state == '+') {
> + struct argv_array name_rev_args = ARGV_ARRAY_INIT;

This struct needs to be cleared to prevent a memory leak.

> +
> + argv_array_pushl(_rev_args, "print-name-rev",
> +  path, sub_sha1, NULL);
> + print_name_rev(name_rev_args.argc, name_rev_args.argv,
> +info->prefix);
> + } else {
> + printf("\n");
> + }
> +}
> +
> +static void status_submodule(const struct cache_entry *list_item, void 
> *cb_data)
> +{
> + struct status_cb *info = cb_data;
> + char *sub_sha1 = xstrdup(oid_to_hex(_item->oid));
> + char *displaypath;
> + struct argv_array diff_files_args = ARGV_ARRAY_INIT;
> +
> + if (!submodule_from_path(null_sha1, list_item->name))
> + die(_("no submodule mapping found in .gitmodules for path 
> '%s'"),
> +   list_item->name);
> +
> + displaypath = get_submodule_displaypath(list_item->name, info->prefix);
> +
> + if (list_item->ce_flags) {
> + print_status(info, 'U', list_item->name,
> +  sha1_to_hex(null_sha1), displaypath);
> + goto cleanup;
> + }
> +
> + if (!is_submodule_active(the_repository, list_item->name)) {
> + print_status(info, '-', list_item->name, sub_sha1, displaypath);
> + goto cleanup;
> + }
> +
> + argv_array_pushl(_files_args, "diff-files",
> +  "--ignore-submodules=dirty", "--quiet", "--",
> +  list_item->name, NULL);
> +
> + /* NEEDSWORK: future optimization possible */

What sort of optimization? maybe you could document that?

> + if (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,
> + info->prefix)) {
> + print_status(info, ' ', list_item->name, sub_sha1, displaypath);
> + } else {
> + if (!info->cached) {
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf sb = STRBUF_INIT;
> +
> + prepare_submodule_repo_env(_array);
> + cp.git_cmd = 1;
> + cp.dir = list_item->name;
> +
> + argv_array_pushl(, "rev-parse",
> +  "--verify", "HEAD", NULL);
> +
> + /* NEEDSWORK: future optimization possible */

Same here.

> + if (capture_command(, , 0))
> + die(_("could not run 'git rev-parse --verify"
> +   "HEAD' in submodule %s"),
> +   list_item->name);
> +
> + strbuf_strip_suffix(, "\n");
> + print_status(info, '+', list_item->name, sb.buf,
> +  displaypath);
> + strbuf_release();
> + } else {
> +   

pre-rebase hook: capture documentation in a <

[Stefan Beller]

From: Jonathan Nieder 

Without this change, the sample hook does not pass a syntax check
(sh -n):

  $ sh -n hooks--pre-rebase.sample
  hooks--pre-rebase.sample: line 101: syntax error near unexpected token `('
  hooks--pre-rebase.sample: line 101: `   merged into it again (either directly 
or indirectly).'

Signed-off-by: Jonathan Nieder 
Improved-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
---

Junio wrote in "What's-cooking":

> ... I do not know how well they are tested
> in the field by people using 'master' in their everyday workflow.
> Ideally, our release process wants to see more people using 'next'
> in their everyday workflow to keep 'master' more stable than any
> tagged release, but I do not have a good idea on how to encourage
> it more than we currently do.

Our internal release of git @ Google is debian experimental,
which is basically the 'next' branch + this patch + another patch.

AFAICT It is a resend of
https://public-inbox.org/git/20120308122105.GA1562@burratino/

As Jonathan is a Debian Developer, it is easy for us to base
our internal version onto debian experimental, but long term we may
want to base our internal version on the original next. :)
To do so, upstream this one last meaningful patch.

The 'another patch' from above is changing and hardcoding
the version number, which we do not want to upstream.

Thanks,
Stefan


 templates/hooks--pre-rebase.sample | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/templates/hooks--pre-rebase.sample 
b/templates/hooks--pre-rebase.sample
index 053f111..b7f81c1 100755
--- a/templates/hooks--pre-rebase.sample
+++ b/templates/hooks--pre-rebase.sample
@@ -88,9 +88,7 @@ else
exit 1
 fi
 
-exit 0
-
-
+<<\DOC_END
 
 This sample hook safeguards topic branches that have been
 published from being rewound.
@@ -167,3 +165,5 @@ To compute (2):
git rev-list master..topic
 
if this is empty, it is fully merged to "master".
+
+DOC_END
-- 
1.8.5.3

Re: [GSoC][PATCH 1/8] submodule--helper: introduce get_submodule_displaypath()

[Brandon Williams]

On 07/11, Prathamesh Chavan wrote:
> Introduce function get_submodule_displaypath() to replace the code
> occurring in submodule_init() for generating displaypath of the
> submodule with a call to it.
> 
> This new function will also be used in other parts of the system
> in later patches.
> 
> Mentored-by: Christian Couder 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
>  builtin/submodule--helper.c | 33 ++---
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 6abdad329..7af4de09b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -220,6 +220,27 @@ static int resolve_relative_url_test(int argc, const 
> char **argv, const char *pr
>   return 0;
>  }
>  
> +static char *get_submodule_displaypath(const char *path, const char *prefix)
> +{
> + const char *super_prefix = get_super_prefix();
> +
> + if (prefix && super_prefix) {
> + BUG("cannot have prefix '%s' and superprefix '%s'",
> + prefix, super_prefix);
> + } else if (prefix) {
> + struct strbuf sb = STRBUF_INIT;
> + char *displaypath = xstrdup(relative_path(path, prefix, ));
> + strbuf_release();
> + return displaypath;
> + } else if (super_prefix) {
> + int len = strlen(super_prefix);
> + const char *format = is_dir_sep(super_prefix[len - 1]) ? "%s%s" 
> : "%s/%s";
> + return xstrfmt(format, super_prefix, path);
> + } else {
> + return xstrdup(path);
> + }
> +}
> +
>  struct module_list {
>   const struct cache_entry **entries;
>   int alloc, nr;
> @@ -339,16 +360,7 @@ static void init_submodule(const char *path, const char 
> *prefix, int quiet)
>  
>   /* Only loads from .gitmodules, no overlay with .git/config */
>   gitmodules_config();
> -
> - if (prefix && get_super_prefix())
> - die("BUG: cannot have prefix and superprefix");
> - else if (prefix)
> - displaypath = xstrdup(relative_path(path, prefix, ));
> - else if (get_super_prefix()) {
> - strbuf_addf(, "%s%s", get_super_prefix(), path);
> - displaypath = strbuf_detach(, NULL);
> - } else
> - displaypath = xstrdup(path);
> + displaypath = get_submodule_displaypath(path, prefix);
>  
>   sub = submodule_from_path(null_sha1, path);
>  
> @@ -363,7 +375,6 @@ static void init_submodule(const char *path, const char 
> *prefix, int quiet)
>* Set active flag for the submodule being initialized
>*/
>   if (!is_submodule_active(the_repository, path)) {
> - strbuf_reset();

Is this line removal intended?  It doesn't look related to the rest of
this patch.

>   strbuf_addf(, "submodule.%s.active", sub->name);
>   git_config_set_gently(sb.buf, "true");
>   }
> -- 
> 2.13.0
> 

-- 
Brandon Williams

[GSoC][PATCH 7/8] diff: change scope of the function count_lines()

[Prathamesh Chavan]

Change the scope of function count_lines for allowing the function
to be reused in other parts of the code as well.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 diff.c | 2 +-
 diff.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 00b4c8669..3240f8283 100644
--- a/diff.c
+++ b/diff.c
@@ -425,7 +425,7 @@ struct emit_callback {
struct strbuf *header;
 };
 
-static int count_lines(const char *data, int size)
+int count_lines(const char *data, int size)
 {
int count, ch, completely_empty = 1, nl_just_seen = 0;
count = 0;
diff --git a/diff.h b/diff.h
index 2d442e296..8522514e9 100644
--- a/diff.h
+++ b/diff.h
@@ -273,6 +273,7 @@ extern struct diff_filepair *diff_unmerge(struct 
diff_options *, const char *pat
 extern int parse_long_opt(const char *opt, const char **argv,
 const char **optarg);
 
+extern int count_lines(const char *data, int size);
 extern int git_diff_basic_config(const char *var, const char *value, void *cb);
 extern int git_diff_heuristic_config(const char *var, const char *value, void 
*cb);
 extern void init_diff_ui_defaults(void);
-- 
2.13.0

[GSoC][PATCH 5/8] submodule: port submodule subcommand 'sync' from shell to C

[Prathamesh Chavan]

Port the submodule subcommand 'sync' from shell to C using the same
mechanism as that used for porting submodule subcommand 'status'.
Hence, here the function cmd_sync() is ported from shell to C.
This is done by introducing three functions: module_sync(),
sync_submodule() and print_default_remote().

The function print_default_remote() is introduced for getting
the default remote as stdout.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 181 +++-
 git-submodule.sh|  56 +-
 2 files changed, 181 insertions(+), 56 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 980b8ed27..4e04b93f3 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -44,6 +44,20 @@ static char *get_default_remote(void)
return ret;
 }
 
+static int print_default_remote(int argc, const char **argv, const char 
*prefix)
+{
+   const char *remote;
+
+   if (argc != 1)
+   die(_("submodule--helper print-default-remote takes no 
arguments"));
+
+   remote = get_default_remote();
+   if (remote)
+   puts(remote);
+
+   return 0;
+}
+
 static int starts_with_dot_slash(const char *str)
 {
return str[0] == '.' && is_dir_sep(str[1]);
@@ -379,6 +393,25 @@ static void module_list_active(struct module_list *list)
*list = active_modules;
 }
 
+static char *get_up_path(const char *path)
+{
+   int i;
+   struct strbuf sb = STRBUF_INIT;
+
+   for (i = count_slashes(path); i; i--)
+   strbuf_addstr(, "../");
+
+   /*
+* Check if 'path' ends with slash or not
+* for having the same output for dir/sub_dir
+* and dir/sub_dir/
+*/
+   if (!is_dir_sep(path[strlen(path) - 1]))
+   strbuf_addstr(, "../");
+
+   return strbuf_detach(, NULL);
+}
+
 static int module_list(int argc, const char **argv, const char *prefix)
 {
int i;
@@ -478,16 +511,18 @@ static void init_submodule(const struct cache_entry 
*list_item, void *cb_data)
char *remote = get_default_remote();
struct strbuf remotesb = STRBUF_INIT;
strbuf_addf(, "remote.%s.url", remote);
-   free(remote);
 
if (git_config_get_string(remotesb.buf, )) {
warning(_("could not lookup configuration '%s'. 
Assuming this repository is its own authoritative upstream."), remotesb.buf);
remoteurl = xgetcwd();
}
relurl = relative_url(remoteurl, url, NULL);
+
+   free(remote);
strbuf_release();
free(remoteurl);
free(url);
+
url = relurl;
}
 
@@ -732,6 +767,148 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct sync_cb {
+   const char *prefix;
+   unsigned int quiet: 1;
+   unsigned int recursive: 1;
+};
+#define SYNC_CB_INIT { NULL, 0, 0 }
+
+static void sync_submodule(const struct cache_entry *list_item, void *cb_data)
+{
+   struct sync_cb *info = cb_data;
+   const struct submodule *sub;
+   char *sub_key, *remote_key;
+   char *sub_origin_url, *super_config_url, *displaypath;
+   struct strbuf sb = STRBUF_INIT;
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   if (!is_submodule_active(the_repository, list_item->name))
+   return;
+
+   sub = submodule_from_path(null_sha1, list_item->name);
+
+   if (!sub || !sub->url)
+   die(_("no url found for submodule path '%s' in .gitmodules"),
+ list_item->name);
+
+   if (starts_with_dot_dot_slash(sub->url) || 
starts_with_dot_slash(sub->url)) {
+   char *remote_url, *up_path;
+   char *remote = get_default_remote();
+   char *remote_key = xstrfmt("remote.%s.url", remote);
+   free(remote);
+
+   if (git_config_get_string(remote_key, _url))
+   remote_url = xgetcwd();
+   up_path = get_up_path(list_item->name);
+   sub_origin_url = relative_url(remote_url, sub->url, up_path);
+   super_config_url = relative_url(remote_url, sub->url, NULL);
+   free(remote_key);
+   free(up_path);
+   free(remote_url);
+   } else {
+   sub_origin_url = xstrdup(sub->url);
+   super_config_url = xstrdup(sub->url);
+   }
+
+   displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+   if (!info->quiet)
+   printf(_("Synchronizing submodule url 

[GSoC][PATCH 3/8] submodule: port set_name_rev() from shell to C

[Prathamesh Chavan]

Function set_name_rev() is ported from git-submodule to the
submodule--helper builtin. The function get_name_rev() generates the
value of the revision name as required, and the function
print_name_rev() handles the formating and printing of the obtained
revision name.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 63 +
 git-submodule.sh| 16 ++--
 2 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e41572f7a..80f744407 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -244,6 +244,68 @@ static char *get_submodule_displaypath(const char *path, 
const char *prefix)
}
 }
 
+static char *get_name_rev(const char *sub_path, const char* object_id)
+{
+   struct strbuf sb = STRBUF_INIT;
+   const char ***d;
+
+   static const char *describe_bare[] = {
+   NULL
+   };
+
+   static const char *describe_tags[] = {
+   "--tags", NULL
+   };
+
+   static const char *describe_contains[] = {
+   "--contains", NULL
+   };
+
+   static const char *describe_all_always[] = {
+   "--all", "--always", NULL
+   };
+
+   static const char **describe_argv[] = {
+   describe_bare, describe_tags, describe_contains,
+   describe_all_always, NULL
+   };
+
+   for (d = describe_argv; *d; d++) {
+   struct child_process cp = CHILD_PROCESS_INIT;
+   prepare_submodule_repo_env(_array);
+   cp.dir = sub_path;
+   cp.git_cmd = 1;
+   cp.no_stderr = 1;
+
+   argv_array_push(, "describe");
+   argv_array_pushv(, *d);
+   argv_array_push(, object_id);
+
+   if (!capture_command(, , 0) && sb.len) {
+   strbuf_strip_suffix(, "\n");
+   return strbuf_detach(, NULL);
+   }
+
+   }
+
+   strbuf_release();
+   return NULL;
+}
+
+static int print_name_rev(int argc, const char **argv, const char *prefix)
+{
+   char *namerev;
+   if (argc != 3)
+   die("print-name-rev only accepts two arguments:  ");
+
+   namerev = get_name_rev(argv[1], argv[2]);
+   if (namerev && namerev[0])
+   printf(" (%s)", namerev);
+   printf("\n");
+
+   return 0;
+}
+
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -1242,6 +1304,7 @@ static struct cmd_struct commands[] = {
{"relative-path", resolve_relative_path, 0},
{"resolve-relative-url", resolve_relative_url, 0},
{"resolve-relative-url-test", resolve_relative_url_test, 0},
+   {"print-name-rev", print_name_rev, 0},
{"init", module_init, SUPPORT_SUPER_PREFIX},
{"remote-branch", resolve_remote_submodule_branch, 0},
{"push-check", push_check, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index e131760ee..e988167e0 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -759,18 +759,6 @@ cmd_update()
}
 }
 
-set_name_rev () {
-   revname=$( (
-   sanitize_submodule_env
-   cd "$1" && {
-   git describe "$2" 2>/dev/null ||
-   git describe --tags "$2" 2>/dev/null ||
-   git describe --contains "$2" 2>/dev/null ||
-   git describe --all --always "$2"
-   }
-   ) )
-   test -z "$revname" || revname=" ($revname)"
-}
 #
 # Show commit summary for submodules in index or working tree
 #
@@ -1042,14 +1030,14 @@ cmd_status()
fi
if git diff-files --ignore-submodules=dirty --quiet -- 
"$sm_path"
then
-   set_name_rev "$sm_path" "$sha1"
+   revname=$(git submodule--helper print-name-rev 
"$sm_path" "$sha1")
say " $sha1 $displaypath$revname"
else
if test -z "$cached"
then
sha1=$(sanitize_submodule_env; cd "$sm_path" && 
git rev-parse --verify HEAD)
fi
-   set_name_rev "$sm_path" "$sha1"
+   revname=$(git submodule--helper print-name-rev 
"$sm_path" "$sha1")
say "+$sha1 $displaypath$revname"
fi
 
-- 
2.13.0

[GSoC][PATCH 8/8] submodule: port submodule subcommand 'summary' from shell to C

[Prathamesh Chavan]

The submodule subcommand 'summary' is ported in the process of
making git-submodule a builtin. The function cmd_summary() from
git-submodule.sh is ported to functions module_summary(),
compute_summary_module_list(), prepare_submodule_summary() and
print_submodule_summary().

The first function module_summary() parses the options of submodule
subcommand and also acts as the front-end of this subcommand.
After parsing them, it calls the compute_summary_module_list()

The functions compute_summary_module_list() runs the diff_cmd,
and generates the modules list, as required by the subcommand.
The generation of this module list is done by the using the
callback function submodule_summary_callback(), and stored in the
structure module_cb.

Once the module list is generated, prepare_submodule_summary()
further goes through the list and filters the list, for
eventually calling the print_submodule_summary() function.

Finally, the print_submodule_summary() takes care of generating
and printing the summary for each submodule.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
This is the first version of submodule-summary posted on the mailing list.

This patch has previously being reviewed off the mailing list as well, and
following are the changes made after the previous update:

An initial check for is_sm_git_dir is added.

Instead of changing the dir to sm_path in a child_process, now we instead
are adding the env_variable GIT_DIR. This helped in avoiding the usage of
shell in the child_process as well for getting all the tests cleared
from t7508-status.

Also, the sentences which earlier were translated unnecessarily were
changed for getting all the test cleared with the env_variable
GETTEXT_POISON, out of which 13 test from t7401-submodule-summary
failed earlier.

Still I'm looking into a better way for generating the abbrevation of
sha1_src and sha1_dst.

Complete build report of this series is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: week-8
Build #129

 builtin/submodule--helper.c | 466 
 git-submodule.sh| 182 +
 2 files changed, 467 insertions(+), 181 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 05d430846..1dc53c2b2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -13,6 +13,9 @@
 #include "remote.h"
 #include "refs.h"
 #include "connect.h"
+#include "revision.h"
+#include "diffcore.h"
+#include "diff.h"
 
 typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
  void *cb_data);
@@ -767,6 +770,468 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct module_cb {
+   unsigned int mod_src;
+   unsigned int mod_dst;
+   struct object_id oid_src;
+   struct object_id oid_dst;
+   char status;
+   const char *sm_path;
+};
+#define MODULE_CB_INIT { 0, 0, NULL, NULL, '\0', NULL }
+
+struct module_cb_list {
+   struct module_cb **entries;
+   int alloc, nr;
+};
+#define MODULE_CB_LIST_INIT { NULL, 0, 0 }
+
+struct summary_cb {
+   int argc;
+   const char **argv;
+   const char *prefix;
+   char *diff_cmd;
+   unsigned int cached: 1;
+   unsigned int for_status: 1;
+   unsigned int quiet: 1;
+   unsigned int files: 1;
+   int summary_limits;
+};
+#define SUMMARY_CB_INIT { 0, NULL, NULL, NULL, 0, 0, 0, 0, 0 }
+
+static void print_submodule_summary(struct summary_cb *info,
+   struct module_cb *p)
+{
+   int missing_src = 0;
+   int missing_dst = 0;
+   char *displaypath;
+   char *sha1_abbr_src;
+   char *sha1_abbr_dst;
+   int errmsg = 0;
+   int total_commits = -1;
+   struct strbuf sb_sha1_src = STRBUF_INIT;
+   struct strbuf sb_sha1_dst = STRBUF_INIT;
+   char *sha1_dst = oid_to_hex(>oid_dst);
+   char *sha1_src = oid_to_hex(>oid_src);
+   char *sm_git_dir = xstrfmt("%s/.git", p->sm_path);
+   int is_sm_git_dir = 0;
+
+   if (!info->cached && !strcmp(sha1_dst, sha1_to_hex(null_sha1))) {
+   if (S_ISGITLINK(p->mod_dst)) {
+   struct child_process cp_rev_parse = CHILD_PROCESS_INIT;
+   struct strbuf sb_rev_parse = STRBUF_INIT;
+
+   cp_rev_parse.git_cmd = 1;
+   cp_rev_parse.no_stderr = 1;
+
+   argv_array_pushf(_rev_parse.env_array,
+"GIT_DIR=%s/.git", p->sm_path);
+   argv_array_pushl(_rev_parse.args,
+"rev-parse", "HEAD", NULL);
+   if (!capture_command(_rev_parse, _rev_parse, 0)) {
+   

[GSoC][PATCH 6/8] submodule: port submodule subcommand 'deinit' from shell to C

[Prathamesh Chavan]

The same mechanism is used even for porting this submodule
subcommand, as used in the ported subcommands till now.
The function cmd_deinit in split up after porting into three
functions: module_deinit(), for_each_submodule_list() and
deinit_submodule().

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 143 
 git-submodule.sh|  55 +
 2 files changed, 144 insertions(+), 54 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4e04b93f3..05d430846 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -909,6 +909,148 @@ static int module_sync(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct deinit_cb {
+   const char *prefix;
+   unsigned int quiet: 1;
+   unsigned int force: 1;
+   unsigned int all: 1;
+};
+#define DEINIT_CB_INIT { NULL, 0, 0, 0 }
+
+static void deinit_submodule(const struct cache_entry *list_item,
+void *cb_data)
+{
+   struct deinit_cb *info = cb_data;
+   const struct submodule *sub;
+   char *displaypath = NULL;
+   struct child_process cp_config = CHILD_PROCESS_INIT;
+   struct strbuf sb_config = STRBUF_INIT;
+   char *sm_path = xstrdup(list_item->name);
+   char *sub_git_dir = xstrfmt("%s/.git", sm_path);
+   struct stat st;
+
+   sub = submodule_from_path(null_sha1, sm_path);
+
+   if (!sub || !sub->name)
+   goto cleanup;
+
+   displaypath = get_submodule_displaypath(sm_path, info->prefix);
+
+   /* remove the submodule work tree (unless the user already did it) */
+   if (is_directory(sm_path)) {
+   /* protect submodules containing a .git directory */
+   if (is_git_directory(sub_git_dir))
+   die(_("Submodule work tree '%s' contains a .git "
+ "directory use 'rm -rf' if you really want "
+ "to remove it including all of its history"),
+ displaypath);
+
+   if (!info->force) {
+   struct child_process cp_rm = CHILD_PROCESS_INIT;
+   cp_rm.git_cmd = 1;
+   argv_array_pushl(_rm.args, "rm", "-qn", sm_path,
+NULL);
+
+   /* list_item->name is changed by cmd_rm() below */
+   if (run_command(_rm))
+   die(_("Submodule work tree '%s' contains local "
+ "modifications; use '-f' to discard 
them"),
+ displaypath);
+   }
+
+   if (!lstat(sm_path, )) {
+   struct strbuf sb_rm = STRBUF_INIT;
+   strbuf_addstr(_rm, sm_path);
+
+   if (!remove_dir_recursively(_rm, 0)) {
+   if (!info->quiet)
+   printf(_("Cleared directory '%s'\n"),
+displaypath);
+   } else {
+   if (!info->quiet)
+   printf(_("Could not remove submodule 
work tree '%s'\n"),
+displaypath);
+   }
+   strbuf_release(_rm);
+   }
+   }
+
+   if (mkdir(sm_path, st.st_mode))
+   die(_("could not create empty submodule directory %s"),
+ displaypath);
+
+   cp_config.git_cmd = 1;
+   argv_array_pushl(_config.args, "config", "--get-regexp", NULL);
+   argv_array_pushf(_config.args, "submodule.%s\\.", sub->name);
+
+   /* remove the .git/config entries (unless the user already did it) */
+   if (!capture_command(_config, _config, 0) && sb_config.len) {
+   char *sub_key = xstrfmt("submodule.%s", sub->name);
+   /*
+* remove the whole section so we have a clean state when
+* the user later decides to init this submodule again
+*/
+   git_config_rename_section_in_file(NULL, sub_key, NULL);
+   if (!info->quiet)
+   printf(_("Submodule '%s' (%s) unregistered for path 
'%s'\n"),
+sub->name, sub->url, displaypath);
+   free(sub_key);
+   }
+
+cleanup:
+   free(displaypath);
+   free(sub_git_dir);
+   free(sm_path);
+   strbuf_release(_config);
+}
+
+static int module_deinit(int argc, const char **argv, const char *prefix)
+{
+   struct deinit_cb info = DEINIT_CB_INIT;
+   struct pathspec pathspec;
+   struct module_list list = 

[GSoC][PATCH 4/8] submodule: port submodule subcommand 'status' from shell to C

[Prathamesh Chavan]

This aims to make git-submodule 'status' a built-in. Hence, the function
cmd_status() is ported from shell to C. This is done by introducing
three functions: module_status(), submodule_status() and print_status().

The function module_status() acts as the front-end of the subcommand.
It parses subcommand's options and then calls the function
module_list_compute() for computing the list of submodules. Then
this functions calls for_each_submodule_list() looping through the
list obtained.

Then for_each_submodule_list() calls submodule_status() for each of the
submodule in its list. The function submodule_status() is responsible
for generating the status each submodule it is called for, and
then calls print_status().

Finally, the function print_status() handles the printing of submodule's
status.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 154 
 git-submodule.sh|  49 +-
 2 files changed, 155 insertions(+), 48 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 80f744407..980b8ed27 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -560,6 +560,159 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct status_cb {
+   const char *prefix;
+   unsigned int quiet: 1;
+   unsigned int recursive: 1;
+   unsigned int cached: 1;
+};
+#define STATUS_CB_INIT { NULL, 0, 0, 0 }
+
+static void print_status(struct status_cb *info, char state, const char *path,
+char *sub_sha1, char *displaypath)
+{
+   if (info->quiet)
+   return;
+
+   printf("%c%s %s", state, sub_sha1, displaypath);
+
+   if (state == ' ' || state == '+') {
+   struct argv_array name_rev_args = ARGV_ARRAY_INIT;
+
+   argv_array_pushl(_rev_args, "print-name-rev",
+path, sub_sha1, NULL);
+   print_name_rev(name_rev_args.argc, name_rev_args.argv,
+  info->prefix);
+   } else {
+   printf("\n");
+   }
+}
+
+static void status_submodule(const struct cache_entry *list_item, void 
*cb_data)
+{
+   struct status_cb *info = cb_data;
+   char *sub_sha1 = xstrdup(oid_to_hex(_item->oid));
+   char *displaypath;
+   struct argv_array diff_files_args = ARGV_ARRAY_INIT;
+
+   if (!submodule_from_path(null_sha1, list_item->name))
+   die(_("no submodule mapping found in .gitmodules for path 
'%s'"),
+ list_item->name);
+
+   displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+   if (list_item->ce_flags) {
+   print_status(info, 'U', list_item->name,
+sha1_to_hex(null_sha1), displaypath);
+   goto cleanup;
+   }
+
+   if (!is_submodule_active(the_repository, list_item->name)) {
+   print_status(info, '-', list_item->name, sub_sha1, displaypath);
+   goto cleanup;
+   }
+
+   argv_array_pushl(_files_args, "diff-files",
+"--ignore-submodules=dirty", "--quiet", "--",
+list_item->name, NULL);
+
+   /* NEEDSWORK: future optimization possible */
+   if (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,
+   info->prefix)) {
+   print_status(info, ' ', list_item->name, sub_sha1, displaypath);
+   } else {
+   if (!info->cached) {
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct strbuf sb = STRBUF_INIT;
+
+   prepare_submodule_repo_env(_array);
+   cp.git_cmd = 1;
+   cp.dir = list_item->name;
+
+   argv_array_pushl(, "rev-parse",
+"--verify", "HEAD", NULL);
+
+   /* NEEDSWORK: future optimization possible */
+   if (capture_command(, , 0))
+   die(_("could not run 'git rev-parse --verify"
+ "HEAD' in submodule %s"),
+ list_item->name);
+
+   strbuf_strip_suffix(, "\n");
+   print_status(info, '+', list_item->name, sb.buf,
+displaypath);
+   strbuf_release();
+   } else {
+   print_status(info, '+', list_item->name, sub_sha1,
+displaypath);
+   }
+   }
+
+   if (info->recursive) {
+   struct child_process cpr = CHILD_PROCESS_INIT;
+
+   cpr.git_cmd = 1;
+   

[GSoC][PATCH 1/8] submodule--helper: introduce get_submodule_displaypath()

[Prathamesh Chavan]

Introduce function get_submodule_displaypath() to replace the code
occurring in submodule_init() for generating displaypath of the
submodule with a call to it.

This new function will also be used in other parts of the system
in later patches.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 33 ++---
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6abdad329..7af4de09b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -220,6 +220,27 @@ static int resolve_relative_url_test(int argc, const char 
**argv, const char *pr
return 0;
 }
 
+static char *get_submodule_displaypath(const char *path, const char *prefix)
+{
+   const char *super_prefix = get_super_prefix();
+
+   if (prefix && super_prefix) {
+   BUG("cannot have prefix '%s' and superprefix '%s'",
+   prefix, super_prefix);
+   } else if (prefix) {
+   struct strbuf sb = STRBUF_INIT;
+   char *displaypath = xstrdup(relative_path(path, prefix, ));
+   strbuf_release();
+   return displaypath;
+   } else if (super_prefix) {
+   int len = strlen(super_prefix);
+   const char *format = is_dir_sep(super_prefix[len - 1]) ? "%s%s" 
: "%s/%s";
+   return xstrfmt(format, super_prefix, path);
+   } else {
+   return xstrdup(path);
+   }
+}
+
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -339,16 +360,7 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 
/* Only loads from .gitmodules, no overlay with .git/config */
gitmodules_config();
-
-   if (prefix && get_super_prefix())
-   die("BUG: cannot have prefix and superprefix");
-   else if (prefix)
-   displaypath = xstrdup(relative_path(path, prefix, ));
-   else if (get_super_prefix()) {
-   strbuf_addf(, "%s%s", get_super_prefix(), path);
-   displaypath = strbuf_detach(, NULL);
-   } else
-   displaypath = xstrdup(path);
+   displaypath = get_submodule_displaypath(path, prefix);
 
sub = submodule_from_path(null_sha1, path);
 
@@ -363,7 +375,6 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 * Set active flag for the submodule being initialized
 */
if (!is_submodule_active(the_repository, path)) {
-   strbuf_reset();
strbuf_addf(, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
}
-- 
2.13.0

[GSoC][PATCH 2/8] submodule--helper: introduce for_each_submodule_list()

[Prathamesh Chavan]

Introduce function for_each_submodule_list() and
replace a loop in module_init() with a call to it.

The new function will also be used in other parts of the
system in later patches.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 39 +--
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7af4de09b..e41572f7a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -14,6 +14,9 @@
 #include "refs.h"
 #include "connect.h"
 
+typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
+ void *cb_data);
+
 static char *get_default_remote(void)
 {
char *dest = NULL, *ret;
@@ -352,17 +355,30 @@ static int module_list(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
-static void init_submodule(const char *path, const char *prefix, int quiet)
+static void for_each_submodule_list(const struct module_list list,
+   submodule_list_func_t fn, void *cb_data)
 {
+   int i;
+   for (i = 0; i < list.nr; i++)
+   fn(list.entries[i], cb_data);
+}
+
+struct init_cb {
+   const char *prefix;
+   unsigned int quiet: 1;
+};
+#define INIT_CB_INIT { NULL, 0 }
+
+static void init_submodule(const struct cache_entry *list_item, void *cb_data)
+{
+   struct init_cb *info = cb_data;
const struct submodule *sub;
struct strbuf sb = STRBUF_INIT;
char *upd = NULL, *url = NULL, *displaypath;
 
-   /* Only loads from .gitmodules, no overlay with .git/config */
-   gitmodules_config();
-   displaypath = get_submodule_displaypath(path, prefix);
+   displaypath = get_submodule_displaypath(list_item->name, info->prefix);
 
-   sub = submodule_from_path(null_sha1, path);
+   sub = submodule_from_path(null_sha1, list_item->name);
 
if (!sub)
die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -374,7 +390,7 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 *
 * Set active flag for the submodule being initialized
 */
-   if (!is_submodule_active(the_repository, path)) {
+   if (!is_submodule_active(the_repository, list_item->name)) {
strbuf_addf(, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
}
@@ -416,7 +432,7 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
if (git_config_set_gently(sb.buf, url))
die(_("Failed to register url for submodule path '%s'"),
displaypath);
-   if (!quiet)
+   if (!info->quiet)
fprintf(stderr,
_("Submodule '%s' (%s) registered for path 
'%s'\n"),
sub->name, url, displaypath);
@@ -445,10 +461,10 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 
 static int module_init(int argc, const char **argv, const char *prefix)
 {
+   struct init_cb info = INIT_CB_INIT;
struct pathspec pathspec;
struct module_list list = MODULE_LIST_INIT;
int quiet = 0;
-   int i;
 
struct option module_init_options[] = {
OPT__QUIET(, N_("Suppress output for initializing a 
submodule")),
@@ -473,8 +489,11 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
if (!argc && git_config_get_value_multi("submodule.active"))
module_list_active();
 
-   for (i = 0; i < list.nr; i++)
-   init_submodule(list.entries[i]->name, prefix, quiet);
+   info.prefix = prefix;
+   info.quiet = !!quiet;
+
+   gitmodules_config();
+   for_each_submodule_list(list, init_submodule, );
 
return 0;
 }
-- 
2.13.0

Re: [PATCH 1/7] api-builtin.txt: document SUPPORT_SUPER_PREFIX

[Brandon Williams]

On 07/10, Martin Ågren wrote:
> Commit 74866d75 ("git: make super-prefix option", 2016-10-07) introduced
> SUPPORT_SUPER_PREFIX as a builtin flag without documenting it in
> api-builtin.txt. The next patch will add another flag, so document
> SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with
> the available flags.
> 
> Signed-off-by: Martin Ågren 
> ---
>  Documentation/technical/api-builtin.txt | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/technical/api-builtin.txt 
> b/Documentation/technical/api-builtin.txt
> index 22a39b929..60f442822 100644
> --- a/Documentation/technical/api-builtin.txt
> +++ b/Documentation/technical/api-builtin.txt
> @@ -42,6 +42,10 @@ where options is the bitwise-or of:
>   on bare repositories.
>   This only makes sense when `RUN_SETUP` is also set.
>  
> +`SUPPORT_SUPER_PREFIX`::
> +
> + The builtin supports --super-prefix.
> +
>  . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
>  
>  Additionally, if `foo` is a new command, there are 3 more things to do:

I added SUPER_PREFIX as an implementation detail when trying to recurse
submodules using multiple processes.  Now that we have a 'struct
repository' my plan is to remove SUPER_PREFIX in its entirety.  Since
this won't happen overnight it may still take a bit till its removed so
maybe it makes sense to better document it until that happens?

Either way I think that this sort of Documention better lives in the
code as it is easier to keep up to date.  Last time I tried to look at
stuff in Documentation/technical the files were either place holders or
completely out of date with what the code did.

-- 
Brandon Williams

What's cooking in git.git (Jul 2017, #03; Mon, 10)

[Junio C Hamano]

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

We also have tons of small updates in preparation for 2.13.3 on
'maint'.  All of them are topics that have been merged to 'master'
more than a few days ago, but I do not know how well they are tested
in the field by people using 'master' in their everyday workflow.
Ideally, our release process wants to see more people using 'next'
in their everyday workflow to keep 'master' more stable than any
tagged release, but I do not have a good idea on how to encourage
it more than we currently do.

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

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

--
[Graduated to "master"]

* ab/sha1dc (2017-07-03) 2 commits
  (merged to 'next' on 2017-07-06 at 5a783032b7)
 + sha1collisiondetection: automatically enable when submodule is populated
 + sha1dc: optionally use sha1collisiondetection as a submodule

 The "collission-detecting" implementation of SHA-1 hash we borrowed
 from is replaced by directly binding the upstream project as our
 submodule.  Glitches on minority platforms are still being worked out.


* ab/wildmatch (2017-06-23) 1 commit
  (merged to 'next' on 2017-07-07 at 34482a9a4f)
 + wildmatch: remove unused wildopts parameter

 Minor code cleanup.


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

 Update the character width tables.


* jk/reflog-walk-maint (2017-07-07) 4 commits
  (merged to 'next' on 2017-07-07 at 611554ba2f)
 + reflog-walk: include all fields when freeing complete_reflogs
 + reflog-walk: don't free reflogs added to cache
 + reflog-walk: duplicate strings in complete_reflogs list
  (merged to 'next' on 2017-07-06 at 7408dd80a1)
 + reflog-walk: skip over double-null oid due to HEAD rename
 (this branch is used by jk/reflog-walk.)

 After "git branch --move" of the currently checked out branch, the
 code to walk the reflog of HEAD via "log -g" and friends
 incorrectly stopped at the reflog entry that records the renaming
 of the branch.


* ks/commit-assuming-only-warning-removal (2017-06-30) 2 commits
  (merged to 'next' on 2017-07-05 at 462a72df95)
 + commit-template: distinguish status information unconditionally
 + commit-template: remove outdated notice about explicit paths

 An old message shown in the commit log template was removed, as it
 has outlived its usefulness.


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

 Typofix.


* pw/unquote-path-in-git-pm (2017-06-30) 4 commits
  (merged to 'next' on 2017-07-05 at 538ab4d599)
 + t9700: add tests for Git::unquote_path()
 + Git::unquote_path(): throw an exception on bad path
 + Git::unquote_path(): handle '\a'
 + add -i: move unquote_path() to Git.pm

 Code refactoring.


* rs/free-and-null (2017-06-29) 1 commit
  (merged to 'next' on 2017-07-06 at 9c9e1d59a2)
 + coccinelle: polish FREE_AND_NULL rules

 Code cleanup.

--
[New Topics]

* kn/ref-filter-branch-list (2017-07-10) 4 commits
  (merged to 'next' on 2017-07-10 at 35fd25c0dd)
 + ref-filter.c: drop return from void function
 + branch: set remote color in ref-filter branch immediately
 + branch: use BRANCH_COLOR_LOCAL in ref-filter format
 + branch: only perform HEAD check for local branches

 The rewrite of "git branch --list" using for-each-ref's internals
 that happened in v2.13 regressed its handling of color.branch.local;
 this has been fixed.

 Will merge to 'master'.


* rs/apply-avoid-over-reading (2017-07-09) 1 commit
  (merged to 'next' on 2017-07-10 at 2d8191ec3f)
 + apply: use strcmp(3) for comparing strings in gitdiff_verify_name()

 Code cleanup.

 Will merge to 'master'.


* rs/progress-overall-throughput-at-the-end (2017-07-09) 1 commit
 - progress: show overall rate in last update

 The progress meter did not give a useful output when we haven't had
 0.5 seconds to measure the throughput during the interval.  Instead
 show the overall throughput rate at the end, which is a much more
 useful number.

 Will merge to 'next'.


* rs/sha1-file-micro-optim (2017-07-09) 2 commits
 - SQUASH???
 - sha1_file: add slash once in for_each_file_in_obj_subdir()

 Code cleanup.

 Perhaps drop.
 cf. 


* rs/urlmatch-cleanup (2017-07-09) 1 commit
  (merged to 'next' on 2017-07-10 at 2dd3e7cab0)
 + urlmatch: use hex2chr() in append_normalized_escapes()

 Code cleanup.

 Will merge to 'master'.


* rs/use-div-round-up (2017-07-10) 1 commit
  (merged to 'next' on 2017-07-10 at 

[GSoC] Update: Week 8

[Prathamesh Chavan]

SUMMARY OF MY PROJECT:

Git submodule subcommands are currently implemented by using shell script
'git-submodule.sh'. There are several reasons why we'll prefer not to
use the shell script. My project intends to convert the subcommands into
C code, thus making them builtins. This will increase Git's portability
and hence the efficiency of working with the git-submodule commands.
Link to the complete proposal: [1]

Mentors:
Stefan Beller 
Christian Couder 

UPDATES:

Following are the updates about my ongoing project:

1. deinit: This patch is updated after its last review.
   and the updated one is attached with this update.

2. summary: Most of the time of the week was utilized for debugging
   this patch. Its debugging is completed and the patch also went
   under some review off the mailing list. Hence, this patch is also
   attached for review in the latest update.

PLAN FOR WEEK-9 (11 July 2017 to 17 July 2017):

1. In this week a new version of 'deinit' patch is included, and well
   as the first version of 'summary' is also included. In the following
   week, I aim to work on improvising these patches.

2. Apart from that, I also aim to work on getting the rest of the patches
   ('status', 'sync', and other functions) merged.

3. There is still work left with the foreach patch, and I wasn't able
   to work on this week. Hence, I will work on finding a way of generating
   the path variable without any hacks.

[1]: 
https://docs.google.com/document/d/1krxVLooWl--75Pot3dazhfygR3wCUUWZWzTXtK1L-xU/

Thanks,
Prathamesh Chavan

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Junio C Hamano]

Martin Ågren  writes:

> Using, e.g., `git -c pager.tag tag -a new-tag` results in errors
> such as "Vim: Warning: Output is not to a terminal" and a garbled
> terminal. A user who makes use of `git tag -a` and `git tag -l`
> will probably choose not to configure `pager.tag` or to set it to
> "no", so that `git tag -a` will actually work, at the cost of not
> getting the pager with `git tag -l`.
>
> In the discussion at [1], it was brought up that 1) the individual
> builtins should be in charge of setting up the paging (as opposed
> to git.c which has no knowledge about what the command is about to
> do) and that 2) there could then be a configuration
> `pager.tag.list` to address the specific problem of `git tag`.
>
> This is an attempt to implement something like that. I decided to
> let `pager.tag.list` fall back to `pager.tag` before falling back
> to "on". The default for `pager.tag` is still "off". I can see how
> that might seem confusing. However, my argument is that it would
> be awkward for `git tag -l` to ignore `pager.tag` -- we are after
> all running a subcommand of `git tag`. Also, this avoids a
> regression for someone who has set `pager.tag` and uses `git tag
> -l`.
>
> I am not moving all builtins to handling the pager on their own,
> but instead introduce a flag IGNORE_PAGER_CONFIG and use it only
> with the tag builtin. That means there's another flag to reason
> about, but it avoids moving all builtins to handling the paging
> themselves just to make one of them do something more "clever".

All of the above smells like taking us in a sensible direction.  I
agree with these design decisions described in the above, i.e.
'pager.tag.list falling back to pager.tag', making this an opt-in to
make code transition easier.

Even though it is purely internal thing, IGNORE_PAGER_CONFIG
probably is a bit confusion-inducing name.  After all, the
subcommand specific configuration is not being ignored---we are
merely delaying our reaction to it---instead of acting on it inside
git.c without giving the subcommand a chance to make a decision, we
are still letting (and we do expect) the subcommand to react to it.

But that is a fairly minor thing we can fix.

> A review would be much appreciated. Comments on the way I
> structured the series would be just as welcome as ones on the
> final result.

I see you CC'ed Peff who's passionate in this area, so these patches
are in good hands already ;-) I briefly skimmed your patches myself,
and did not spot anything glaringly wrong.

Thanks.

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Brandon Williams]

On 07/10, Jeff King wrote:
> On Sun, Jul 09, 2017 at 10:05:49AM -0700, Junio C Hamano wrote:
> 
> > René Scharfe  writes:
> > 
> > > I wonder when we can begin to target C99 in git's source, though. :)
> > 
> > Let's get the ball rolling by starting to use some of the useful
> > features like designated initializers, perhaps, in a small, critical
> > and reasonably stable part of the system that anybody must compile,
> > leave it in one full release cycle or two, and when we hear nobody
> > complains, introduce it en masse for the remainder of the system?
> > 
> > That way, we will see if there are people who need pre-C99 soon
> > enough, and we won't have to scramble reverting too many changes
> > when it happens.
> 
> Neat idea. Something like this?
> 
> -- >8 --
> Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT
> 
> There are certain C99 features that might be nice to use in
> our code base, but we've hesitated to do so in order to
> avoid breaking compatibility with older compilers. But we
> don't actually know if people are even using pre-C99
> compilers these days.
> 
> One way to figure that out is to introduce a very small use
> of a feature, and see if anybody complains. The strbuf code
> is a good place to do this for a few reasons:
> 
>   - it always gets compiled, no matter which Makefile knobs
> have been tweaked.
> 
>   - it's very stable; this definition hasn't changed in a
> long time and is not likely to (so if we have to revert,
> it's unlikely to cause headaches)
> 
> If this patch can survive a few releases without complaint,
> then we can feel more confident that designated initializers
> are widely supported by our user base.  It also is an
> indication that other C99 features may be supported, but not
> a guarantee (e.g., gcc had designated initializers before
> C99 existed).
> 
> And if we do get complaints, then we'll have gained some
> data and we can easily revert this patch.
> 
> Signed-off-by: Jeff King 
> ---
> I suspected we could also do something with __STDC_VERSION__, though I
> wonder what compilers set it to when not in standards-compliant mode.
> gcc-6 claims C11 when no specific -std flag is given.
> 
> And obviously before releasing this or anything similar, it would be
> nice to see results from people building pu. I'm especially curious
> whether MSVC would work with this (or if people even still use it, since
> Git for Windows is pretty mature?).
> 
>  strbuf.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/strbuf.h b/strbuf.h
> index 2075384e0..e705b94db 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -68,7 +68,7 @@ struct strbuf {
>  };
>  
>  extern char strbuf_slopbuf[];
> -#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
> +#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }

I love that this is happening!  And maybe someday soon we can do:

  for (int i = 0; i < n; i++)

So that we can scope loop variables to the loops themselves.

-- 
Brandon Williams

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

[Stefan Beller]

On Thu, Jul 6, 2017 at 3:39 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
>>> index 0a639664fd..1fa01210a2 100644
>>> --- a/Documentation/git-push.txt
>>> +++ b/Documentation/git-push.txt
>>> @@ -212,8 +212,9 @@ must not already exist.
>>>  +
>>>  Note that all forms other than `--force-with-lease=:`
>>>  that specifies the expected current value of the ref explicitly are
>>> -still experimental and their semantics may change as we gain experience
>>
>> This indicates that this feature is not 'experimental' any more, but disabled
>> (for safety reasons as described below). This implies we will not change the
>> heuristic for push.allowLazyForceWithLease easily.
>
> I actually wanted to say it was a failed experiment, but I see your
> point.  Let's leave the "still experimental" label.
>

After rethinking this feature and how to make it safer, we could actually
*ask* the user to confirm the sha1:

# implicit lease:
$ git push --force-with-lease 
# either do an implicit fetch for the refspec first
# or use the remote tracking branch:
This would lose HEAD=27956ac767, including
the following commits on  :
27956ac767 Merge branch 'js/rebase-i-final' into pu
a1b1c5eb04 Merge branch 'sb/hashmap-cleanup' into pu
... and 13 more
Confirm to lose commits by typing yes: yes
... normal push

But that may be more effort than this patch originally intended to be,
but I would think this makes the lease effective.

Downside is the I/O (Have we any command that is taking
user input as such? -p option for reset/add may come to mind)
and the unfriendlyness to scripts, but scripting may rely on the
non-lazy form of leases.

[PATCH 7/7] tag: make git tag -l use pager by default

[Martin Ågren]

The previous patch introduced `pager.tag.list`. Its default was to use
the value of `pager.tag` or, if that was also not set, to fall back to
"off".

Change that fallback value to "on". Let the default value for
`pager.tag` remain at "off". Update documentation and tests.

Signed-off-by: Martin Ågren 
---
 Documentation/git-tag.txt |  2 +-
 builtin/tag.c |  2 +-
 t/t7006-pager.sh  | 12 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 6ad5811a2..fbdfb3f59 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -103,7 +103,7 @@ as `--contains` is provided. See the documentation for each 
of those
 options for details.
 +
 When determining whether to use a pager, `pager.tag.list` is considered
-before `pager.tag`.
+before `pager.tag`. If neither is set, the default is to use a pager.
 See linkgit:git-config[1].
 
 --sort=::
diff --git a/builtin/tag.c b/builtin/tag.c
index e96ef7d70..ec69fca61 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -447,7 +447,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
if (cmdmode == 'l')
-   setup_auto_pager("tag.list", 0);
+   setup_auto_pager("tag.list", 1);
setup_auto_pager("tag", 0);
 
if (keyid) {
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index ed34c6734..94df89a5f 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,22 +134,22 @@ test_expect_success TTY 'configuration can enable pager 
(from subdir)' '
}
 '
 
-test_expect_success TTY 'git tag -l defaults to not paging' '
+test_expect_success TTY 'git tag -l defaults to paging' '
rm -f paginated.out &&
test_terminal git tag -l &&
-   ! test -e paginated.out
+   test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects pager.tag' '
rm -f paginated.out &&
-   test_terminal git -c pager.tag tag -l &&
-   test -e paginated.out
+   test_terminal git -c pager.tag=false tag -l &&
+   ! test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects pager.tag.list' '
rm -f paginated.out &&
-   test_terminal git -c pager.tag=false -c pager.tag.list tag -l &&
-   test -e paginated.out
+   test_terminal git -c pager.tag -c pager.tag.list=false tag -l &&
+   ! test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects --no-pager' '
-- 
2.13.2.653.gfb5159d

[PATCH 6/7] tag: make git tag -l consider new config `pager.tag.list`

[Martin Ågren]

Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as
"Vim: Warning: Output is not to a terminal" and a garbled terminal. A
user who makes use of `git tag -a` and `git tag -l` will probably choose
not to configure `pager.tag` or to set it to "no", so that `git tag -a`
will actually work, at the cost of not getting the pager with `git tag
-l`.

Introduce `pager.tag.list`. Teach git tag to prefer it over `pager.tag`
when running with -l. Update the documentation and add tests. Update an
existing test to use `pager.tag.list` instead of `pager.tag` together
with `git tag -l` since the former is arguably more relevant.

Do not introduce an "else" where we call setup_auto_pager(), although we
could have. Omitting it might keep someone who later implements even
more fine-grained configurations from building a correspondingly
complicated decision tree which carefully ensures that
setup_auto_pager() is called precisely once. A greedy approach such as
the one taken here works just fine.

Noticed-by: Anatoly Borodin 
Suggested-by: Jeff King 
Signed-off-by: Martin Ågren 
---
 Documentation/git-tag.txt |  4 
 builtin/tag.c |  2 ++
 t/t7006-pager.sh  | 16 +++-
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 1eb15afa1..6ad5811a2 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -101,6 +101,10 @@ patterns may be given; if any of them matches, the tag is 
shown.
 This option is implicitly supplied if any other list-like option such
 as `--contains` is provided. See the documentation for each of those
 options for details.
++
+When determining whether to use a pager, `pager.tag.list` is considered
+before `pager.tag`.
+See linkgit:git-config[1].
 
 --sort=::
Sort based on the key given.  Prefix `-` to sort in
diff --git a/builtin/tag.c b/builtin/tag.c
index e0f129872..e96ef7d70 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -446,6 +446,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
+   if (cmdmode == 'l')
+   setup_auto_pager("tag.list", 0);
setup_auto_pager("tag", 0);
 
if (keyid) {
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 43cce3694..ed34c6734 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -146,9 +146,15 @@ test_expect_success TTY 'git tag -l respects pager.tag' '
test -e paginated.out
 '
 
+test_expect_success TTY 'git tag -l respects pager.tag.list' '
+   rm -f paginated.out &&
+   test_terminal git -c pager.tag=false -c pager.tag.list tag -l &&
+   test -e paginated.out
+'
+
 test_expect_success TTY 'git tag -l respects --no-pager' '
rm -f paginated.out &&
-   test_terminal git -c pager.tag --no-pager tag -l &&
+   test_terminal git -c pager.tag.list --no-pager tag -l &&
! test -e paginated.out
 '
 
@@ -166,6 +172,14 @@ test_expect_success TTY 'git tag -a respects pager.tag' '
test -e paginated.out
 '
 
+test_expect_success TTY 'git tag -a ignores pager.tag.list' '
+   test_when_finished "git tag -d newtag" &&
+   rm -f paginated.out &&
+   test_terminal git -c pager.tag -c pager.tag.list=false \
+   tag -am message newtag &&
+   test -e paginated.out
+'
+
 test_expect_success TTY 'git tag -a respects --paginate' '
test_when_finished "git tag -d newtag" &&
rm -f paginated.out &&
-- 
2.13.2.653.gfb5159d

[PATCH 5/7] tag: handle `pager.tag`-configuration within the builtin

[Martin Ågren]

Use the mechanisms introduced in two earlier patches to ignore
`pager.tag` in git.c and let the `git tag` builtin handle it on its own.

This is in preparation for the next patch, where we will want to handle
slightly different configuration variables depending on which options
are used with `git tag`. For this reason, place the call to
setup_auto_pager() after the options have been parsed.

No functional change is intended. That said, there is a window between
where the pager is started before and after this patch, and if an error
occurs within this window, as of this patch the error message might not
be paged where it would have been paged before. Since
operation-parsing has to happen inside this window, a difference can be
seen with, e.g., `git -c pager.tag="echo pager is used" tag
--unknown-option`. This change in paging-behavior should be acceptable
since it only affects erroneous usages.

Signed-off-by: Martin Ågren 
---
 builtin/tag.c | 2 ++
 git.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 01154ea8d..e0f129872 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -446,6 +446,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
+   setup_auto_pager("tag", 0);
+
if (keyid) {
opt.sign = 1;
set_signing_key(keyid);
diff --git a/git.c b/git.c
index 696eaf87a..4d05452a3 100644
--- a/git.c
+++ b/git.c
@@ -489,7 +489,7 @@ static struct cmd_struct commands[] = {
{ "stripspace", cmd_stripspace },
{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | 
SUPPORT_SUPER_PREFIX},
{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
-   { "tag", cmd_tag, RUN_SETUP },
+   { "tag", cmd_tag, RUN_SETUP | IGNORE_PAGER_CONFIG },
{ "unpack-file", cmd_unpack_file, RUN_SETUP },
{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
{ "update-index", cmd_update_index, RUN_SETUP },
-- 
2.13.2.653.gfb5159d

[PATCH 2/7] git.c: let builtins opt for handling `pager.foo` themselves

[Martin Ågren]

Before launching a builtin git foo and unless mechanisms with precedence
are in use, we check for and handle the `pager.foo` config. This is done
without considering exactly how git foo is being used, and indeed, git.c
cannot (and should not) know what the arguments to git foo are supposed
to achieve.

In practice this means that, e.g., `git -c pager.tag tag -a new-tag`
results in errors such as "Vim: Warning: Output is not to a terminal"
and a garbled terminal. A user who makes use of `git tag -a` and `git
tag -l` will probably choose not to configure `pager.tag` or to set it
to "no", so that `git tag -a` will actually work, at the cost of not
getting the pager with `git tag -l`.

To allow individual builtins to make more informed decisions about when
to respect `pager.foo`, introduce a flag IGNORE_PAGER_CONFIG. If the flag
is set, do not check `pager.foo`. This applies to two code-paths -- one
in run_builtin() and one in execv_dashed_external().

Document the flag in api-builtin.txt.

Don't add any users of IGNORE_PAGER_CONFIG just yet, one will follow in a
later patch.

Suggested-by: Jeff King 
Signed-off-by: Martin Ågren 
---
 Documentation/technical/api-builtin.txt |  6 ++
 git.c   | 14 --
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-builtin.txt 
b/Documentation/technical/api-builtin.txt
index 60f442822..61fd8eeb2 100644
--- a/Documentation/technical/api-builtin.txt
+++ b/Documentation/technical/api-builtin.txt
@@ -46,6 +46,12 @@ where options is the bitwise-or of:
 
The builtin supports --super-prefix.
 
+`IGNORE_PAGER_CONFIG`::
+
+   Ignore the `pager.`-configuration in git.c, instead
+   giving the builtin the chance to handle it and possibly
+   more fine-grained configurations (`pager..`).
+
 . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
 
 Additionally, if `foo` is a new command, there are 3 more things to do:
diff --git a/git.c b/git.c
index 489aab4d8..ae92f8ed5 100644
--- a/git.c
+++ b/git.c
@@ -283,6 +283,12 @@ static int handle_alias(int *argcp, const char ***argv)
  */
 #define NEED_WORK_TREE (1<<3)
 #define SUPPORT_SUPER_PREFIX   (1<<4)
+/*
+ * Ignore the `pager.`-configuration, instead giving the builtin
+ * the chance to handle it and possibly more fine-grained
+ * configurations (`pager..`).
+ */
+#define IGNORE_PAGER_CONFIG(1<<5)
 
 struct cmd_struct {
const char *cmd;
@@ -306,7 +312,8 @@ static int run_builtin(struct cmd_struct *p, int argc, 
const char **argv)
prefix = setup_git_directory_gently(_ok);
}
 
-   if (use_pager == -1 && p->option & (RUN_SETUP | 
RUN_SETUP_GENTLY))
+   if (use_pager == -1 && p->option & (RUN_SETUP | 
RUN_SETUP_GENTLY) &&
+   !(p->option & IGNORE_PAGER_CONFIG))
use_pager = check_pager_config(p->cmd);
if (use_pager == -1 && p->option & USE_PAGER)
use_pager = 1;
@@ -543,11 +550,14 @@ static void execv_dashed_external(const char **argv)
 {
struct child_process cmd = CHILD_PROCESS_INIT;
int status;
+   struct cmd_struct *builtin;
 
if (get_super_prefix())
die("%s doesn't support --super-prefix", argv[0]);
 
-   if (use_pager == -1)
+   builtin = get_builtin(argv[0]);
+   if (use_pager == -1 &&
+   !(builtin && builtin->option & IGNORE_PAGER_CONFIG))
use_pager = check_pager_config(argv[0]);
commit_pager_choice();
 
-- 
2.13.2.653.gfb5159d

[PATCH 4/7] t7006: add tests for how git tag paginates

[Martin Ågren]

Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as
"Vim: Warning: Output is not to a terminal" and a garbled terminal. A
user who makes use of `git tag -a` and `git tag -l` will probably choose
not to configure `pager.tag` or to set it to "no", so that `git tag -a`
will actually work, at the cost of not getting the pager with `git tag
-l`.

Since we're about to add some finer granularity to the configuration,
add tests around how git tag respects `pager.tag` and how that
configuration is ignored if --no-pager or --paginate are used.

Construct tests with two different subcommands: using -l and using -a,
where -a is being used essentially as a representative for "not -l".

Make one of the tests demonstrate the behavior mentioned above, where
`git tag -a` respects `pager.tag`. Actually, the tests use `git tag -a`
with -m, in which case no editor is launched, but that is irrelevant,
since we just want to see whether the pager is used or not. (If `git tag
-am` ever learns to avoid the pager, these tests will need to be
updated and two of them will fail.)

Signed-off-by: Martin Ågren 
---
 t/t7006-pager.sh | 40 
 1 file changed, 40 insertions(+)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 20b4d83c2..43cce3694 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,6 +134,46 @@ test_expect_success TTY 'configuration can enable pager 
(from subdir)' '
}
 '
 
+test_expect_success TTY 'git tag -l defaults to not paging' '
+   rm -f paginated.out &&
+   test_terminal git tag -l &&
+   ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -l respects pager.tag' '
+   rm -f paginated.out &&
+   test_terminal git -c pager.tag tag -l &&
+   test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -l respects --no-pager' '
+   rm -f paginated.out &&
+   test_terminal git -c pager.tag --no-pager tag -l &&
+   ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a defaults to not paging' '
+   test_when_finished "git tag -d newtag" &&
+   rm -f paginated.out &&
+   test_terminal git tag -am message newtag &&
+   ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a respects pager.tag' '
+   test_when_finished "git tag -d newtag" &&
+   rm -f paginated.out &&
+   test_terminal git -c pager.tag tag -am message newtag &&
+   test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a respects --paginate' '
+   test_when_finished "git tag -d newtag" &&
+   rm -f paginated.out &&
+   test_terminal git -c pager.tag=false --paginate \
+   tag -am message newtag &&
+   test -e paginated.out
+'
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {
-- 
2.13.2.653.gfb5159d

[PATCH 3/7] git.c: provide setup_auto_pager()

[Martin Ågren]

The previous patch introduced a way for builtins to declare that they
will take responsibility for handling the `pager.foo`-config item. (See
the commit message of that patch for why that could be useful.)

Provide setup_auto_pager(), which builtins can call in order to handle
`pager.`, including possibly starting the pager. Make this function
don't do anything if a pager has already been started.

When the `cmd` given to setup_auto_pager() contains one or more '.', use
a fallback strategy which checks `pager.foo.bar.baz`, then
`pager.foo.bar`, then `pager.foo`, then resorts to the provided default
value. This ensures a consistent fallback strategy for builtins which
take this type of more fine-grained pager configuration.

An alternative fallback strategy would have been to check for
`pager.foo.bar.baz`, then immediately fall back to `def`. However, that
would have meant that git foo would sometimes completely ignore
`pager.foo`, which seems conceptually wrong. It would also have meant
that builtins that are moved to more fine-grained pager configurations
would have regressed for certain usecases.

Whenever this function is called from a builtin, git.c will already have
called commit_pager_choice(). Since commit_pager_choice() treats the
special value -1 as "punt" or "not yet decided", it is not a problem
that we might end up calling commit_pager_choice() many times. Make the
new function use -1 in the same way and document it as "punt".

Don't add any users of setup_auto_pager just yet, one will follow in
later patches.

setup_auto_pager() is more capable than it needs to be for this patch
series. It would have been sufficient to handle zero or one '.'. We
would probably have wanted some verification+BUG()-patterns or to
define whether we split at the first or last '.', so it seems just as
easy, and certainly more flexible, to handle the more general situation.

Signed-off-by: Martin Ågren 
---
 builtin.h | 14 ++
 git.c | 28 
 2 files changed, 42 insertions(+)

diff --git a/builtin.h b/builtin.h
index 498ac80d0..a6ed6c4ac 100644
--- a/builtin.h
+++ b/builtin.h
@@ -25,6 +25,20 @@ struct fmt_merge_msg_opts {
 extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
 struct fmt_merge_msg_opts *);
 
+/**
+ * If a builtin has IGNORE_PAGER_CONFIG set, the builtin should call this early
+ * when it wishes to respect the `pager.foo`-config. In the simplest case, the
+ * `cmd` is the name of the builtin, e.g., "foo". If a paging-choice has 
already
+ * been setup, this does nothing. The default in `def` should be 0 for "pager
+ * off", 1 for "pager on" or -1 for "punt".
+ *
+ * With one or more '.', substrings are tried out from longer to shorter. If no
+ * config is found, uses `def`. For example, with `cmd` as "foo.bar.baz", this
+ * function tries `pager.foo.bar.baz`, `pager.foo.bar` and `pager.foo` in that
+ * order before resorting to `def`.
+ */
+extern void setup_auto_pager(const char *cmd, int def);
+
 extern int is_builtin(const char *s);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index ae92f8ed5..696eaf87a 100644
--- a/git.c
+++ b/git.c
@@ -33,6 +33,34 @@ static void commit_pager_choice(void) {
}
 }
 
+void setup_auto_pager(const char *cmd, int def)
+{
+   if (use_pager != -1)
+   return;
+
+   use_pager = check_pager_config(cmd);
+
+   if (use_pager == -1) {
+   struct strbuf buf = STRBUF_INIT;
+   size_t len;
+
+   strbuf_addstr(, cmd);
+   len = buf.len;
+   while (use_pager == -1 && len--) {
+   if (buf.buf[len] == '.') {
+   strbuf_setlen(, len);
+   use_pager = check_pager_config(buf.buf);
+   }
+   }
+   strbuf_release();
+   }
+
+   if (use_pager == -1)
+   use_pager = def;
+
+   commit_pager_choice();
+}
+
 static int handle_options(const char ***argv, int *argc, int *envchanged)
 {
const char **orig_argv = *argv;
-- 
2.13.2.653.gfb5159d

[PATCH 1/7] api-builtin.txt: document SUPPORT_SUPER_PREFIX

[Martin Ågren]

Commit 74866d75 ("git: make super-prefix option", 2016-10-07) introduced
SUPPORT_SUPER_PREFIX as a builtin flag without documenting it in
api-builtin.txt. The next patch will add another flag, so document
SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with
the available flags.

Signed-off-by: Martin Ågren 
---
 Documentation/technical/api-builtin.txt | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/technical/api-builtin.txt 
b/Documentation/technical/api-builtin.txt
index 22a39b929..60f442822 100644
--- a/Documentation/technical/api-builtin.txt
+++ b/Documentation/technical/api-builtin.txt
@@ -42,6 +42,10 @@ where options is the bitwise-or of:
on bare repositories.
This only makes sense when `RUN_SETUP` is also set.
 
+`SUPPORT_SUPER_PREFIX`::
+
+   The builtin supports --super-prefix.
+
 . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
 
 Additionally, if `foo` is a new command, there are 3 more things to do:
-- 
2.13.2.653.gfb5159d

[PATCH 0/7] tag: more fine-grained pager-configuration

[Martin Ågren]

Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as "Vim: 
Warning: Output is not to a terminal" and a garbled terminal. A user who makes 
use of `git tag -a` and `git tag -l` will probably choose not to configure 
`pager.tag` or to set it to "no", so that `git tag -a` will actually work, at 
the cost of not getting the pager with `git tag -l`.

In the discussion at [1], it was brought up that 1) the individual builtins 
should be in charge of setting up the paging (as opposed to git.c which has no 
knowledge about what the command is about to do) and that 2) there could then 
be a configuration `pager.tag.list` to address the specific problem of `git 
tag`.

This is an attempt to implement something like that. I decided to let 
`pager.tag.list` fall back to `pager.tag` before falling back to "on". The 
default for `pager.tag` is still "off". I can see how that might seem 
confusing. However, my argument is that it would be awkward for `git tag -l` to 
ignore `pager.tag` -- we are after all running a subcommand of `git tag`. Also, 
this avoids a regression for someone who has set `pager.tag` and uses `git tag 
-l`.

I am not moving all builtins to handling the pager on their own, but instead 
introduce a flag IGNORE_PAGER_CONFIG and use it only with the tag builtin. That 
means there's another flag to reason about, but it avoids moving all builtins 
to handling the paging themselves just to make one of them do something more 
"clever".

The discussion mentioned above discusses various approaches. It also notes how 
the current pager.foo-configuration conflates _whether_ and _how_ to run a 
pager. Arguably, this series paints ourselves even further into that corner. 
The idea of `pager.foo.command` and `pager.foo.enabled` has been mentioned and 
this series might make such an approach slightly less clean, conceptually. We 
could introduce `paging.foo` as a "yes"/"no"/"maybe" to go alongside 
`pager.foo` which is then "less"/"cat"/ That's definitely outside this 
series, but should not be prohibited by it...

A review would be much appreciated. Comments on the way I structured the series 
would be just as welcome as ones on the final result.

Martin

[1] https://public-inbox.org/git/nrmbrl$hsk$1...@blaine.gmane.org/T/

Martin Ågren (7):
  api-builtin.txt: document SUPPORT_SUPER_PREFIX
  git.c: let builtins opt for handling `pager.foo` themselves
  git.c: provide setup_auto_pager()
  t7006: add tests for how git tag paginates
  tag: handle `pager.tag`-configuration within the builtin
  tag: make git tag -l consider new config `pager.tag.list`
  tag: make git tag -l use pager by default

 Documentation/git-tag.txt   |  4 +++
 Documentation/technical/api-builtin.txt | 10 ++
 builtin.h   | 14 +
 builtin/tag.c   |  4 +++
 git.c   | 44 +--
 t/t7006-pager.sh| 54 +
 6 files changed, 127 insertions(+), 3 deletions(-)

-- 
2.13.2.653.gfb5159d

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Stefan Beller  writes:

> On Mon, Jul 10, 2017 at 9:44 AM, Junio C Hamano  wrote:
>
> Credit goes to Brandon for spotting it, but the introduction of
> "trailing comma at the end of enum definition is allowed in c99"
> is e1327023ea (grep: refactor the concept of "grep source" into
> an object, 2012-02-02) IMHO, which is more time that proved this
> feature being supported on all compilers.

Yup, I did dig down to that commit earlier when I wrote

  https://public-inbox.org/git/xmqqlgolm2jk@gitster.mtv.corp.google.com/

I just forgot about it, as very many things going on the list ;-)

Thanks.

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Johannes Sixt  writes:

>> I am not sure what negative impact you think the macro-ness would
>> have to the validity of the result from this test balloon.  An old
>> compiler that does not understand designated initializer syntax
>> would fail to compile both the same way, no?
>>
>>  struct strbuf buf0 = STRBUF_INIT;
>>  struct strbuf buf1 = { .alloc = 0, .len = 0, .buf = strbuf_slopbuf };
>
> I said it is uninteresting, not that there is a negative impact. There
> is simply nothing gained for strbuf users: They would use STRBUF_INIT
> before and after the change and would not benefit from designated
> initializers.
>
> This change may serve well as a test balloon, but not as an example of
> the sort of changes that we would want to see later (of the kind
> "change FOO_INIT macro to use designated initializers"; they are just
> code churn).

Oh, absolutely.

Here is another possible test balloon, that may actually be useful
as an example.  I think there is a topic in flight that touches this
array, unfortunately, so I probably would find another one that is
more stable for a real follow-up patch to the one from Peff.

 diff.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/diff.c b/diff.c
index 00b4c86698..b3864a2e03 100644
--- a/diff.c
+++ b/diff.c
@@ -47,15 +47,15 @@ static long diff_algorithm;
 static unsigned ws_error_highlight_default = WSEH_NEW;
 
 static char diff_colors[][COLOR_MAXLEN] = {
-   GIT_COLOR_RESET,
-   GIT_COLOR_NORMAL,   /* CONTEXT */
-   GIT_COLOR_BOLD, /* METAINFO */
-   GIT_COLOR_CYAN, /* FRAGINFO */
-   GIT_COLOR_RED,  /* OLD */
-   GIT_COLOR_GREEN,/* NEW */
-   GIT_COLOR_YELLOW,   /* COMMIT */
-   GIT_COLOR_BG_RED,   /* WHITESPACE */
-   GIT_COLOR_NORMAL,   /* FUNCINFO */
+   [DIFF_RESET] = GIT_COLOR_RESET,
+   [DIFF_CONTEXT] = GIT_COLOR_NORMAL,
+   [DIFF_METAINFO] = GIT_COLOR_BOLD,
+   [DIFF_FRAGINFO] = GIT_COLOR_CYAN,
+   [DIFF_FILE_OLD] = GIT_COLOR_RED,
+   [DIFF_FILE_NEW] = GIT_COLOR_GREEN,
+   [DIFF_COMMIT] = GIT_COLOR_YELLOW,
+   [DIFF_WHITESPACE] = GIT_COLOR_BG_RED,
+   [DIFF_FUNCINFO] = GIT_COLOR_NORMAL,
 };
 
 static NORETURN void die_want_option(const char *option_name)

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 10.07.2017 um 22:38 schrieb Junio C Hamano:

Johannes Sixt  writes:


It's a pity, though, that you do not suggest something even more
useful, such as C++14.


I cannot tell if this part is tongue-in-cheek (especially the "++"),
so I will ignore it to avoid wasting time.


Actually, I'm serious.


Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT



-#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
+#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }


While this may serve as a test balloon, changing STRBUF_INIT, or any
of those _INIT macros, is actually the least interesting. The
interesting instances are initializations for which we do *not* have a
macro.


I am not sure what negative impact you think the macro-ness would
have to the validity of the result from this test balloon.  An old
compiler that does not understand designated initializer syntax
would fail to compile both the same way, no?

struct strbuf buf0 = STRBUF_INIT;
struct strbuf buf1 = { .alloc = 0, .len = 0, .buf = strbuf_slopbuf };


I said it is uninteresting, not that there is a negative impact. There 
is simply nothing gained for strbuf users: They would use STRBUF_INIT 
before and after the change and would not benefit from designated 
initializers.


This change may serve well as a test balloon, but not as an example of 
the sort of changes that we would want to see later (of the kind "change 
FOO_INIT macro to use designated initializers"; they are just code churn).


-- Hannes

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Johannes Sixt  writes:

> It's a pity, though, that you do not suggest something even more
> useful, such as C++14.

I cannot tell if this part is tongue-in-cheek (especially the "++"),
so I will ignore it to avoid wasting time.

>> Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT
>
>> -#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
>> +#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
>
> While this may serve as a test balloon, changing STRBUF_INIT, or any
> of those _INIT macros, is actually the least interesting. The
> interesting instances are initializations for which we do *not* have a
> macro.

I am not sure what negative impact you think the macro-ness would
have to the validity of the result from this test balloon.  An old
compiler that does not understand designated initializer syntax
would fail to compile both the same way, no?

struct strbuf buf0 = STRBUF_INIT;
struct strbuf buf1 = { .alloc = 0, .len = 0, .buf = strbuf_slopbuf };

Bug: Git checkout case Insensitive branch name compare

[kinchit raja]

Bugs Details:

Git checkout case Insensitive branch name compare

Steps to Reproduce:

 I have a remote branch which is as shown below.
Example: feature_12345_write.netCodeForSomeFeature

 I used the git command on git bash and typed
"feature_12345_write.NetCodeForSomeFeature" thinking I am mapping
"feature_12345_write.netCodeForSomeFeature" locally.

So after I got the branch locally, I tried git pull and I see the following
"Your configuration specifies to merge with the ref
'refs/heads/feature_12345_write.NetCodeForSomeFeature' from the
remote, but no such ref was fetched.

 Although when I did "git log", it shows proper history for
"feature_12345_write.netCodeForSomeFeature" branch.

However when I use the command "git pull --rebase origin branch" it
says "fatal: Couldn't find remote ref
feature_12345_write.NetCodeForSomeFeature"
Because the remote branch has 'n' instead of 'N' in '.net' for branch name.

This is because I used "git checkout branch" instead of "git checkout
branch origin/remote branch name" since the local branch name I wanted
was the same as the remote branch name.

I don't see this in other branches.

I think the command is not considering case sensitivity of characters
in the branch name.

Thanks.

-- 
Best Regards,
Kinchit Raja

Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize

[Ramsay Jones]

On 10/07/17 18:44, Jeff King wrote:
> On Mon, Jul 10, 2017 at 10:35:24AM -0700, Junio C Hamano wrote:
> 
>> Jeff King  writes:
>>
>>> You can already build and test with ASan by doing:
>>>
>>>   make CFLAGS=-fsanitize=address test
>>>
>>> but there are a few slight annoyances:
>>>
>>>   1. It's a little long to type.
>>>
>>>   2. It override your CFLAGS completely. You'd probably
>>>  still want -O2, for instance.
>>>
>>>   3. It's a good idea to also turn off "recovery", which
>>>  lets the program keep running after a problem is
>>>  detected (with the intention of finding as many bugs as
>>>  possible in a given run). Since Git's test suite should
>>>  generally run without triggering any problems, it's
>>>  better to abort immediately and fail the test when we
>>>  do find an issue.
>>
>> Unfortunately I do not think Comparing between versions in
>> https://gcc.gnu.org/onlinedocs, it appears that -fsanitize-recover
>> is not configurable for folks still with GCC 4.x series, and this
>> patch is not very useful unless you disable the recovery for the
>> purpose of running our tests as you said X-<.
> 
> I didn't actually dig into the history of gcc support at all. Back in
> the 4.x time-frame I tried using ASan and couldn't get it to work at
> all. I ended up just always building with clang (which from my
> mostly-ignorant view seems to to be the primary platform for ASan
> development).
> 
> Since this is an optional build that doesn't need to be available
> everywhere, I'd actually be fine with saying "just use clang". But as
> far as I can tell, gcc seems to work fine these days. I consider this
> mostly a best-effort tool.
> 
> I'm also not sure of the behavior without -fno-sanitize-recover. I think
> ASan may barf either way. The commit message for my config.mak from a
> year or two ago claims that the problem was actually with UBSan. It
> would be useful in the long run for that to work, too.

Just FYI, I had a quick look at this tonight. I applied your
patches to master, the tried 'make SANITIZE=address test', which
worked fine. I then tried 'make SANITIZE=undefined test' and I had
to control+C it after nearly two hours on one test! ;-) (somewhere
in the t4xxx - unfortunately I overwrote the output file without
thinking).

[BTW I am on Linux Mint 18.2 x86_64, gcc version 5.4.0]

After a quick look at the ./t-basic.sh test, I managed to get
the test to complete (with 15 tests failing), with the following
patch applied:

-- >8 --
diff --git a/Makefile b/Makefile
index 3c341b2a6..8e6433738 100644
--- a/Makefile
+++ b/Makefile
@@ -1016,7 +1016,7 @@ ifdef SANITIZE
 BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
 BASIC_CFLAGS += -fno-omit-frame-pointer
 ifeq ($(SANITIZE),undefined)
-BASIC_CFLAGS += -DNO_UNALIGNED_LOADS
+BASIC_CFLAGS += -DNO_UNALIGNED_LOADS -DSHA1DC_FORCE_ALIGNED_ACCESS
 endif
 endif
 
diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index 25eded139..3baddc636 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -118,6 +118,10 @@
 #define SHA1DC_ALLOW_UNALIGNED_ACCESS
 #endif /*UNALIGNMENT DETECTION*/
 
+#if defined(SHA1DC_ALLOW_UNALIGNED_ACCESS) && 
defined(SHA1DC_FORCE_ALIGNED_ACCESS)
+#undef SHA1DC_ALLOW_UNALIGNED_ACCESS
+#endif
+
 
 #define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n
 #define rotate_left(x,n)  (((x)<<(n))|((x)>>(32-(n


Hmm, hopefully that is not whitespace damaged.

ATB,
Ramsay Jones

Re: [PATCH 4/4] hook: add a simple first example

[Junio C Hamano]

Kaartic Sivaraam  writes:

>  I made an attempt to make the second example work with amending 
>  with the aim of making it suitable for usage out of the box. It
>  seems that it's not easy to make it work as the status of a file
>  cannot be determined correctly when the index while amending
>  introduces changes to a file that has a change in the commit being
>  amended.
>
>  Is there any way in which the second example could be made to work with
>  amending without much effort? I'm asking this assuming something might
>  have happened, since the script was added, that could ease the task.

Sorry, but I do not understand what you are asking here.

Ahh, do you mean if we can avoid doing one half of the 1/4 (i.e. the
part that removes the commented out 'diff --name-status') and instead
make it a useful example (while still removing the thing that
comments out the "conflicts:")?

After going back and checking 1/4, I realize that I misread the patch.
you did keep the commented out 'diff --name-status' thing, so it still
has three---it just lost one half of the original "first" example.  So
please disregard my earlier "do we still have three, not two?"



>  Documentation/githooks.txt | 3 +++
>  templates/hooks--prepare-commit-msg.sample | 5 -
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index fdc01aa25..59f38efba 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -121,6 +121,9 @@ it is not suppressed by the `--no-verify` option.  A 
> non-zero exit
>  means a failure of the hook and aborts the commit.  It should not
>  be used as replacement for pre-commit hook.
>  
> +The sample `prepare-commit-msg` hook that comes with Git removes the
> +help message found in the commented portion of the commit template.
> +
>  commit-msg
>  ~~
>  
> diff --git a/templates/hooks--prepare-commit-msg.sample 
> b/templates/hooks--prepare-commit-msg.sample
> index a15d6d634..a84c3e5a8 100755
> --- a/templates/hooks--prepare-commit-msg.sample
> +++ b/templates/hooks--prepare-commit-msg.sample
> @@ -9,7 +9,8 @@
>  #
>  # To enable this hook, rename this file to "prepare-commit-msg".
>  
> -# This hook includes three examples.
> +# This hook includes three examples.  The first one removes the
> +# "# Please enter the commit message..." help message.
>  #
>  # The second includes the output of "git diff --name-status -r"
>  # into the message, just before the "git status" output.  It is
> @@ -23,6 +24,8 @@ COMMIT_MSG_FILE=$1
>  COMMIT_SOURCE=$2
>  SHA1=$3
>  
> +@PERL_PATH@ -i.bak -ne 'print unless(m/^. Please enter the commit 
> message/..m/^#$/)' "$COMMIT_MSG_FILE"
> +
>  # case "$COMMIT_SOURCE,$SHA1" in
>  #  ,|template,)
>  #@PERL_PATH@ -i.bak -pe '

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 10.07.2017 um 09:03 schrieb Jeff King:

On Sun, Jul 09, 2017 at 10:05:49AM -0700, Junio C Hamano wrote:


René Scharfe  writes:


I wonder when we can begin to target C99 in git's source, though. :)


Let's get the ball rolling...


Good to know that you do not resist moving to a more modern build 
environment.



by starting to use some of the useful
features like designated initializers,


It's a pity, though, that you do not suggest something even more useful, 
such as C++14.



Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT



-#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
+#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }


While this may serve as a test balloon, changing STRBUF_INIT, or any of 
those _INIT macros, is actually the least interesting. The interesting 
instances are initializations for which we do *not* have a macro.


-- Hannes

Re: [PATCH 3/4] hook: add signature using "interpret-trailers"

[Junio C Hamano]

Ramsay Jones  writes:

>> It works well in all cases except when the user invokes
>> "git commit" without any arguments. In that case manually
>> add a new line after the first line to ensure it's consistent
>> with the output of "-s" option.
>> 
>
> Again, s/signature/sign-off/g, or similar (including subject line).

Thanks for being a careful reader.

>> Signed-off-by: Kaartic Sivaraam 
>> ---
>>  templates/hooks--prepare-commit-msg.sample | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/templates/hooks--prepare-commit-msg.sample 
>> b/templates/hooks--prepare-commit-msg.sample
>> index 708f0e92c..a15d6d634 100755
>> --- a/templates/hooks--prepare-commit-msg.sample
>> +++ b/templates/hooks--prepare-commit-msg.sample
>> @@ -32,4 +32,8 @@ SHA1=$3
>>  # esac
>>  
>>  # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: 
>> \1/p')
>> -# grep -qs "^$SOB" "$COMMIT_MSG_FILE" || echo "$SOB" >> "$COMMIT_MSG_FILE"
>> +# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
>> +# if test -z "$COMMIT_SOURCE"
>> +# then
>> +#   @PERL_PATH@ -i.bak -pe 'print "\n" if($first_line++ == 0)' 
>> "$COMMIT_MSG_FILE"
>> +# fi

I think we should do

print "\n" if !$first_line++

for brevity (and also avoid "if(" that lacks SP) here.

Re: [PATCH 2/4] hook: name the positional variables

[Junio C Hamano]

Kaartic Sivaraam  writes:

> It's always nice to have named variables instead of
> positional variables as they communicate their purpose
> well.
>
> Appropriately name the positional variables of the hook
> to make it easier to see what's going on.
>
> Signed-off-by: Kaartic Sivaraam 
> ---

Makes sense.  Thanks.

Re: [PATCH 1/4] hook: cleanup script

[Junio C Hamano]

Kaartic Sivaraam  writes:

> Prepare the 'preare-commit-msg' sample script for
> upcoming changes. Preparation includes removal of
> an example that has outlived it's purpose. The example
> is the one that comments the "Conflicts:" part of a
> merge commit message. It isn't relevant anymore as
> it's done by default since 261f315b ("merge & sequencer:
> turn "Conflicts:" hint into a comment", 2014-08-28).
>
> Further remove the relevant comments from the sample script
> and update the documentation.
>
> Signed-off-by: Kaartic Sivaraam 
> ---
>  Documentation/githooks.txt |  3 ---
>  templates/hooks--prepare-commit-msg.sample | 20 
>  2 files changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 706091a56..fdc01aa25 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -121,9 +121,6 @@ it is not suppressed by the `--no-verify` option.  A 
> non-zero exit
>  means a failure of the hook and aborts the commit.  It should not
>  be used as replacement for pre-commit hook.
>  
> -The sample `prepare-commit-msg` hook that comes with Git comments
> -out the `Conflicts:` part of a merge's commit message.
> -
>  commit-msg
>  ~~

Makes sense.

>  
> diff --git a/templates/hooks--prepare-commit-msg.sample 
> b/templates/hooks--prepare-commit-msg.sample
> index 86b8f227e..b8ba335cf 100755
> --- a/templates/hooks--prepare-commit-msg.sample
> +++ b/templates/hooks--prepare-commit-msg.sample
> @@ -9,8 +9,7 @@
>  #
>  # To enable this hook, rename this file to "prepare-commit-msg".
>  
> -# This hook includes three examples.  The first comments out the
> -# "Conflicts:" part of a merge commit.
> +# This hook includes three examples.

Didn't we just remove one, reducing the total number to two?  If so,
the second and the third below would need to be promoted to the
first and the second, I think.

>  # The second includes the output of "git diff --name-status -r"
>  # into the message, just before the "git status" output.  It is
> @@ -20,17 +19,14 @@
>  # The third example adds a Signed-off-by line to the message, that can
>  # still be edited.  This is rarely a good idea.
>  
> -case "$2,$3" in
> -  merge,)
> -@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; 
> print' "$1" ;;
>  
> -# ,|template,)
> -#   @PERL_PATH@ -i.bak -pe '
> -#  print "\n" . `git diff --cached --name-status -r`
> -# if /^#/ && $first++ == 0' "$1" ;;
> -
> -  *) ;;
> -esac
> +# case "$2,$3" in
> +#  ,|template,)
> +#@PERL_PATH@ -i.bak -pe '
> +#   print "\n" . `git diff --cached --name-status -r`
> +# if /^#/ && $first++ == 0' "$1" ;;
> +#  *) ;;
> +# esac
>  
>  # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: 
> \1/p')
>  # grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"

[PATCH] ref-filter.c: drop return from void function

[Alejandro R . Sedeño]

Sun's C compiler errors out on this pattern:

void foo() { ... }
void bar() { return foo(); }

Signed-off-by: Alejandro R. Sedeño 
---
 ref-filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 2cc7b01..467c027 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -221,7 +221,7 @@ static void objectname_atom_parser(struct used_atom *atom, 
const char *arg)
 
 static void refname_atom_parser(struct used_atom *atom, const char *arg)
 {
-   return refname_atom_parser_internal(>u.refname, arg, atom->name);
+   refname_atom_parser_internal(>u.refname, arg, atom->name);
 }
 
 static align_type parse_align_position(const char *s)
-- 
2.1.4

Re: [PATCH] l10n: de.po: fix typo

[Junio C Hamano]

Ralf Thielow  writes:

> Reported-by: Andre Hinrichs 
> Signed-off-by: Ralf Thielow 
> ---
> Hi Andre,
>
> thanks for the bugreport!
>
> Ralf

Thanks.  Let me take it directly to 'maint'.

Re: [PATCH/RFC] rebase: make resolve message clearer for inexperienced users

[William Duclot]

Junio C Hamano writes:
> William Duclot  writes:
> 
> > diff --git a/git-rebase.sh b/git-rebase.sh
> > index 2cf73b88e..50457f687 100755
> > --- a/git-rebase.sh
> > +++ b/git-rebase.sh
> > @@ -55,9 +55,10 @@ LF='
> >  '
> >  ok_to_skip_pre_rebase=
> >  resolvemsg="
> > -$(gettext 'When you have resolved this problem, run "git rebase 
> > --continue".
> > -If you prefer to skip this patch, run "git rebase --skip" instead.
> > -To check out the original branch and stop rebasing, run "git rebase 
> > --abort".')
> > +$(gettext 'Resolve this conflict manually, mark it as resolved with "git 
> > add ",
> > +then run "git rebase --continue".
> > +You can instead skip this commit: run "git rebase --skip".
> > +To stop the whole rebasing and get back to your pre-rebase state, run "git 
> > rebase --abort".')
> >  "
> 
> I find the updated one easier to follow in general.
> Disecting the phrases in the above:
> 
>  - The original said "When you have resolved this problem", without
>giving a guidance how to resolve, and without saying what the
>problem is.  The updated one says "conflict" to clarify the
>"problem", and suggests "git add" as the tool to use after a
>manual resolition.  
> 
>Modulo that there are cases where "git rm" is the right tool, the
>updated one is strict improvement.

I also wrote "" when there could be several. Maybe
'mark it as resolved with "git add/rm"' would be a better (and shorter)
formulation?

>  - The original said "to check out the original branch and stop
>rebasing", and the updated one says "to stop and get back to",
>which is in a more logical order.  
> 
>"the whole rebasing" used as a noun feels something is missing
>there, though.  I wonder if "To get back to the state before you
>started 'rebase -i', run 'git rebase --abort'" is sufficient,
>without saying anything further about abandoning the rebase in
>progress (i.e. "and stop rebasing" or "stop the whole rebasing").

Definitely seems clearer to me: straight to the point.

> Thanks.

Happy to see this patch seems interesting to you. I feel like a lot of
git messages could be improved this way to offer a UI more welcoming to
inexperienced user (which is a *broad* segment of users). But I am not
aware of the cost of translation of this kind of patch: would several
patches like this one be welcomed?

Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize

[Junio C Hamano]

Jeff King  writes:

> On Mon, Jul 10, 2017 at 10:35:24AM -0700, Junio C Hamano wrote:
>
>> Jeff King  writes:
>> 
>> > You can already build and test with ASan by doing:
>> >
>> >   make CFLAGS=-fsanitize=address test
>> >
>> > but there are a few slight annoyances:
>> >
>> >   1. It's a little long to type.
>> >
>> >   2. It override your CFLAGS completely. You'd probably
>> >  still want -O2, for instance.
>> >
>> >   3. It's a good idea to also turn off "recovery", which
>> >  lets the program keep running after a problem is
>> >  detected (with the intention of finding as many bugs as
>> >  possible in a given run). Since Git's test suite should
>> >  generally run without triggering any problems, it's
>> >  better to abort immediately and fail the test when we
>> >  do find an issue.
>> 
>> Unfortunately I do not think Comparing between versions in
>> https://gcc.gnu.org/onlinedocs, it appears that -fsanitize-recover
>> is not configurable for folks still with GCC 4.x series, and this
>> patch is not very useful unless you disable the recovery for the
>> purpose of running our tests as you said X-<.
>
> I didn't actually dig into the history of gcc support at all. Back in
> the 4.x time-frame I tried using ASan and couldn't get it to work at
> all. I ended up just always building with clang (which from my
> mostly-ignorant view seems to to be the primary platform for ASan
> development).
>
> Since this is an optional build that doesn't need to be available
> everywhere, I'd actually be fine with saying "just use clang". But as
> far as I can tell, gcc seems to work fine these days. I consider this
> mostly a best-effort tool.
>
> I'm also not sure of the behavior without -fno-sanitize-recover. I think
> ASan may barf either way. The commit message for my config.mak from a
> year or two ago claims that the problem was actually with UBSan. It
> would be useful in the long run for that to work, too.

Yes.  I'd agree with all of the above.  While copyediting my
response, I somehow ended up removing one paragraph before that
"Unfortunately" by accident X-<, but the paragraph said essentially
the same "this is optional so it is a strict improvement, and I do
agree recovery must be disabled to be useful in our context".

Sorry for a possible confusion.

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Ben Peart]

On 7/10/2017 12:04 PM, Jeff King wrote:

On Mon, Jul 10, 2017 at 10:57:57AM -0400, Ben Peart wrote:


If this patch can survive a few releases without complaint,
then we can feel more confident that designated initializers
are widely supported by our user base.  It also is an
indication that other C99 features may be supported, but not
a guarantee (e.g., gcc had designated initializers before
C99 existed).


Correct.  MSVC also supports designated initializers but does not fully
support C99.


Out of curiosity, does MSVC define __STDC_VERSION__, and if so, to what?


It does not. While it does support a subset of C99's features, it 
doesn't support the entirety of that language so defining 
__STDC_VERSION__ would be inaccurate and misleading.





And obviously before releasing this or anything similar, it would be
nice to see results from people building pu. I'm especially curious
whether MSVC would work with this (or if people even still use it, since
Git for Windows is pretty mature?).


We do use MSVC internally as that gives us access to the great debuggers and
profilers on the Windows platform.  Fortunately, this particular C99
construct _is_ supported by MSVC.  I applied the patch below and complied it
with both MSVC and gcc for Windows and both builds succeeded.


Thanks. This kind of prompt testing and response is very appreciated. It
is unfortunate if we have to pick and choose C99-isms rather than using
the whole thing as a base. But that's probably just reality.

-Peff

Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize

[Jeff King]

On Mon, Jul 10, 2017 at 10:35:24AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > You can already build and test with ASan by doing:
> >
> >   make CFLAGS=-fsanitize=address test
> >
> > but there are a few slight annoyances:
> >
> >   1. It's a little long to type.
> >
> >   2. It override your CFLAGS completely. You'd probably
> >  still want -O2, for instance.
> >
> >   3. It's a good idea to also turn off "recovery", which
> >  lets the program keep running after a problem is
> >  detected (with the intention of finding as many bugs as
> >  possible in a given run). Since Git's test suite should
> >  generally run without triggering any problems, it's
> >  better to abort immediately and fail the test when we
> >  do find an issue.
> 
> Unfortunately I do not think Comparing between versions in
> https://gcc.gnu.org/onlinedocs, it appears that -fsanitize-recover
> is not configurable for folks still with GCC 4.x series, and this
> patch is not very useful unless you disable the recovery for the
> purpose of running our tests as you said X-<.

I didn't actually dig into the history of gcc support at all. Back in
the 4.x time-frame I tried using ASan and couldn't get it to work at
all. I ended up just always building with clang (which from my
mostly-ignorant view seems to to be the primary platform for ASan
development).

Since this is an optional build that doesn't need to be available
everywhere, I'd actually be fine with saying "just use clang". But as
far as I can tell, gcc seems to work fine these days. I consider this
mostly a best-effort tool.

I'm also not sure of the behavior without -fno-sanitize-recover. I think
ASan may barf either way. The commit message for my config.mak from a
year or two ago claims that the problem was actually with UBSan. It
would be useful in the long run for that to work, too.

-Peff

Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize

[Junio C Hamano]

Jeff King  writes:

> You can already build and test with ASan by doing:
>
>   make CFLAGS=-fsanitize=address test
>
> but there are a few slight annoyances:
>
>   1. It's a little long to type.
>
>   2. It override your CFLAGS completely. You'd probably
>  still want -O2, for instance.
>
>   3. It's a good idea to also turn off "recovery", which
>  lets the program keep running after a problem is
>  detected (with the intention of finding as many bugs as
>  possible in a given run). Since Git's test suite should
>  generally run without triggering any problems, it's
>  better to abort immediately and fail the test when we
>  do find an issue.

Unfortunately I do not think Comparing between versions in
https://gcc.gnu.org/onlinedocs, it appears that -fsanitize-recover
is not configurable for folks still with GCC 4.x series, and this
patch is not very useful unless you disable the recovery for the
purpose of running our tests as you said X-<.

> With this patch, all of that happens automatically when you
> run:
>
>   make SANITIZE=address test
>
> Signed-off-by: Jeff King 
> ---
>  Makefile | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 9c9c42f8f..59f6bdcd7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1012,6 +1012,10 @@ ifdef DEVELOPER
>  CFLAGS += $(DEVELOPER_CFLAGS)
>  endif
>  
> +ifdef SANITIZE
> +BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
> +endif
> +
>  ifndef sysconfdir
>  ifeq ($(prefix),/usr)
>  sysconfdir = /etc

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Stefan Beller]

On Mon, Jul 10, 2017 at 9:44 AM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>>> That way, we will see if there are people who need pre-C99 soon
>>> enough, and we won't have to scramble reverting too many changes
>>> when it happens.
>>
>> Neat idea. Something like this?
>
> Yes, your log message said everything I wanted to say, including
> possiblity that some compilers may have specific features without
> supporting all of c99.
>
> We accidentally started using "trailing comma at the end of enum
> definition is allowed in c99", and we know it has been safe at least
> for a cycle.  Credits goes to Brandon for 4538eef5 ("grep: add
> submodules as a grep source type", 2016-12-16).

Credit goes to Brandon for spotting it, but the introduction of
"trailing comma at the end of enum definition is allowed in c99"
is e1327023ea (grep: refactor the concept of "grep source" into
an object, 2012-02-02) IMHO, which is more time that proved this
feature being supported on all compilers.

Thanks for getting the ball rolling, just wondering if the patch needs
a comment in the code. The commit message is very thorough though.

Thanks,
Stefan

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Andreas Schwab]

On Jul 10 2017, Jeff King  wrote:

> If this patch can survive a few releases without complaint,
> then we can feel more confident that designated initializers
> are widely supported by our user base.  It also is an
> indication that other C99 features may be supported, but not
> a guarantee (e.g., gcc had designated initializers before
> C99 existed).

Note that the GNU C designated initializers initially used a different
syntax than the one C99 adopted.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: What's cooking in git.git (Jul 2017, #01; Wed, 5)

[Stefan Beller]

On Thu, Jul 6, 2017 at 7:13 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Speaking of submodules, It's not just features, but I also send bug fixes. ;)
>> https://public-inbox.org/git/20170630003851.17288-1-sbel...@google.com/
>> (That patch is not related to this series, except for working in the 
>> submodule
>> area, but I consider that patch more important than e.g. this series.)
>
> I did not see the patch as fixing a bug, though.
>
> I do agree that overwriting the branch tips in the submodule
> repositories, possibly rewinding and discarding user's work done on
> the local branches, is indeed a problem.  It however is unclear why
> detaching HEAD is a good solution to solve that problem.

I am not saying detaching a HEAD is a good solution, but I am saying
it is a better solution than corrupting the submodule branch, such
that commits are lost in the submodule, only to be recorvered via the
reflog.

> After all, there must have been a reason why the user had checked
> out a branch and had pointed it at a specific commit (presumably,
> so that further work would be done while on the branch, to make it
> easier and safer to eventually push the result back to the upstream
> of the submodule's project).  So another solution that seems equally
> viable, if not even more so, could be to fail the recursive checkout
> saying why the checkout cannot be done, just like we fail a checkout
> when a local change interferes with updating the contents in the
> working tree and the index with an error message explaining which
> paths are problematic.

That seems like a better model to me for now.

> I am *not* saying which one among the above two is better; I am not
> even saying that there could be only these two possible solutions.
> I just found the posted patch unsatisfactory because it did not make
> it clear why the chosen solution is a good one.

ok. My long term plan is to introduce another type of symbolic ref,
which references a gitlink in another repository, such that the submodule
can have a clear distinction between "follows the superproject",
"has its own authoritative branch" and "its detached HEAD can mean
anything, e.g. historical submodule behavior"

> Perhaps I misread the description; but that would mean the
> description was prone to be misread and has room for improvement ;-)

ok.

Re: [PATCH 2/5] test-lib: turn on ASan abort_on_error by default

[Junio C Hamano]

Jeff King  writes:

> By default, ASan will exit with code 1 when it sees an
> error. This means we'll notice a problem when we expected
> git to succeed, but not in a test_must_fail block.
>
> Let's ask it to actually raise SIGABRT instead. That will
> give us a signal death that test_must_fail will notice. As a
> bonus, it may also leave a coredump, which can be handy for
> digging into a failure.

Well thought-out.  Thanks.

> Signed-off-by: Jeff King 
> ---
>  t/test-lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 961194a50..1b6e53f78 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -41,7 +41,7 @@ GIT_BUILD_DIR="$TEST_DIRECTORY"/..
>  # the noise level. This needs to happen at the start of the script,
>  # before we even do our "did we build git yet" check (since we don't
>  # want that one to complain to stderr).
> -: ${ASAN_OPTIONS=detect_leaks=0}
> +: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
>  export ASAN_OPTIONS
>  
>  

Re: [PATCH] use DIV_ROUND_UP

[René Scharfe]

Am 10.07.2017 um 09:27 schrieb Jeff King:

On Sat, Jul 08, 2017 at 12:35:35PM +0200, René Scharfe wrote:

diff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c
index 2dc9c82ecf..06c479f70a 100644
--- a/ewah/ewah_bitmap.c
+++ b/ewah/ewah_bitmap.c
@@ -210,8 +210,8 @@ size_t ewah_add(struct ewah_bitmap *self, eword_t word)
  void ewah_set(struct ewah_bitmap *self, size_t i)
  {
const size_t dist =
-   (i + BITS_IN_EWORD) / BITS_IN_EWORD -
-   (self->bit_size + BITS_IN_EWORD - 1) / BITS_IN_EWORD;
+   DIV_ROUND_UP(i + 1, BITS_IN_EWORD) -
+   DIV_ROUND_UP(self->bit_size, BITS_IN_EWORD);


...this first one is a bit trickier. Our "n" in the first one is now
"i+1".  But that's because the original was implicitly canceling the
"-1" and "+1" terms.

So I think it's a true mechanical conversion, but I have to admit the
original is confusing. Without digging I suspect it's correct, though,
just because a simple bug here would mean that our ewah bitmaps totally
don't work. So it's probably not worth spending time on.


A few lines below there is "self->bit_size = i + 1", so the code
calculates how many words the old and the new value are apart (or by how
many words the bitmap needs to be extended), which becomes easier to see
with the patch.

René

Re: [PATCH] doc: correct a mistake in an illustration

[Junio C Hamano]

Kaartic Sivaraam  writes:

> The first illustration of the "RECOVERING FROM UPSTREAM REBASE"
> section in the 'git-rebase' documentation wasn't in line with the
> rest of the illustrations of that section.
>
> Correct it.

Yeah. It is unclear from the lack of context, but this part shows
the original state with some number of commits on the 'master'
branch, and the illustrations that follow depict the evolution of
histories starting from this state, yet they have one less commit
on their 'master' branch (and they are not trying to say the master
is reset to lose one commit), which is what "wasn't in line" in the
log message means.  

Looks correct to me.  Thanks.

>
> Signed-off-by: Kaartic Sivaraam 
> ---
>  Documentation/git-rebase.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 53f4e1444..652a99062 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -675,7 +675,7 @@ on this 'subsystem'.  You might end up with a history 
> like the
>  following:
>  
>  
> -o---o---o---o---o---o---o---o---o  master
> +o---o---o---o---o---o---o---o  master
>\
> o---o---o---o---o  subsystem
>  \

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Jeff King  writes:

>> That way, we will see if there are people who need pre-C99 soon
>> enough, and we won't have to scramble reverting too many changes
>> when it happens.
>
> Neat idea. Something like this?

Yes, your log message said everything I wanted to say, including
possiblity that some compilers may have specific features without
supporting all of c99.

We accidentally started using "trailing comma at the end of enum
definition is allowed in c99", and we know it has been safe at least
for a cycle.  Credits goes to Brandon for 4538eef5 ("grep: add
submodules as a grep source type", 2016-12-16).

> -- >8 --
> Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT
>
> There are certain C99 features that might be nice to use in
> our code base, but we've hesitated to do so in order to
> avoid breaking compatibility with older compilers. But we
> don't actually know if people are even using pre-C99
> compilers these days.
>
> One way to figure that out is to introduce a very small use
> of a feature, and see if anybody complains. The strbuf code
> is a good place to do this for a few reasons:
>
>   - it always gets compiled, no matter which Makefile knobs
> have been tweaked.
>
>   - it's very stable; this definition hasn't changed in a
> long time and is not likely to (so if we have to revert,
> it's unlikely to cause headaches)
>
> If this patch can survive a few releases without complaint,
> then we can feel more confident that designated initializers
> are widely supported by our user base.  It also is an
> indication that other C99 features may be supported, but not
> a guarantee (e.g., gcc had designated initializers before
> C99 existed).
>
> And if we do get complaints, then we'll have gained some
> data and we can easily revert this patch.
>
> Signed-off-by: Jeff King 
> ---
> I suspected we could also do something with __STDC_VERSION__, though I
> wonder what compilers set it to when not in standards-compliant mode.
> gcc-6 claims C11 when no specific -std flag is given.
>
> And obviously before releasing this or anything similar, it would be
> nice to see results from people building pu. I'm especially curious
> whether MSVC would work with this (or if people even still use it, since
> Git for Windows is pretty mature?).
>
>  strbuf.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/strbuf.h b/strbuf.h
> index 2075384e0..e705b94db 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -68,7 +68,7 @@ struct strbuf {
>  };
>  
>  extern char strbuf_slopbuf[];
> -#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
> +#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
>  
>  /**
>   * Life Cycle Functions

Performance improvement for git pull rebase and autostash

[neuling]

Sorry, the max characters per line restriction of the e-mail broke the 
workflow.

Here it is again. 

  git pull rebase = true or preserve
   |
   |
   |
   |
   |
check if local branch has no new commits
   |
 no|   yes
  -+- 
  | |
  | |
  | |
   rebase validation  ff merge validation for 
   for local changes  conflicting local changes 
  | |
no changes|  changes no conflicts   |has conflicts 
  +-+--
  ||| |
  ||| |
  ||| |
do rebase use autostash? do ff merge use autostash?
  ||| |
  |no  |  yes   |no   |  yes
  |+-   |-+-
  |||   || |
  |||   || |
  |   conflict error   stash changes|  conflict error   stash changes
  |||   || |
  |||   || |
  ||do rebase   ||   ff merge
  |||   || |
  |||   || |
  ||pop stash   ||  pop stash
  |||   || |
  |||   || |
success  abort   success success   abort  success



Regarda,
Mattias




Von:Mattias Neuling/DAKOSY/DE
An: git@vger.kernel.org
Datum:  10.07.2017 18:09
Betreff:Performance improvement for git pull rebase and autostash 



Hi,

I have some suggestions to improve performance of 'git pull --rebase'.

1. If I have no new local commits "git pull --rebase" will do a fast 
forward merge. But if I have changes to local files I have to stash them 
also if they are not affected by the new commits from origin. I think in 
that case git should not reject changes to every local file and has to use 

the fast forward merge validation instead.

2. If I have no changes to local files and I use 'git pull --rebase 
--autostash' no stashing should take place.


The improved workflow would look like as follows.


 git pull rebase = true or preserve
   |
   |
   |
   |
   |
check if local branch has no new commits
   |
 no|   yes
  -+- 
  | |
  | |
  | |
   rebase validation  ff merge validation for 
   for local changes  conflicting local changes 
  | |
no changes|  changes no conflicts   |has conflicts 
  +-+--
  ||| |
  ||| |
  ||| |
do rebase use autostash? do ff merge use autostash?
  ||| |
  |no  |  yes   |no   |  yes
  |+-   |-+-
  |||   || |
  |||   || |
  |   conflict error   stash changes|  conflict error stash 
changes
  |||   || |
  |||   || |
  ||do rebase   ||do ff 
merge
  ||

Re: [PATCH/RFC] rebase: make resolve message clearer for inexperienced users

[Junio C Hamano]

William Duclot  writes:

> diff --git a/git-rebase.sh b/git-rebase.sh
> index 2cf73b88e..50457f687 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -55,9 +55,10 @@ LF='
>  '
>  ok_to_skip_pre_rebase=
>  resolvemsg="
> -$(gettext 'When you have resolved this problem, run "git rebase --continue".
> -If you prefer to skip this patch, run "git rebase --skip" instead.
> -To check out the original branch and stop rebasing, run "git rebase 
> --abort".')
> +$(gettext 'Resolve this conflict manually, mark it as resolved with "git add 
> ",
> +then run "git rebase --continue".
> +You can instead skip this commit: run "git rebase --skip".
> +To stop the whole rebasing and get back to your pre-rebase state, run "git 
> rebase --abort".')
>  "

I find the updated one easier to follow in general.
Disecting the phrases in the above:

 - The original said "When you have resolved this problem", without
   giving a guidance how to resolve, and without saying what the
   problem is.  The updated one says "conflict" to clarify the
   "problem", and suggests "git add" as the tool to use after a
   manual resolition.  

   Modulo that there are cases where "git rm" is the right tool, the
   updated one is strict improvement.

 - The original said "If you prefer to skip" and the updated one
   says "You can instead skip".  Neither gives any guidance to
   decide when it is the right thing to skip, but probably that is
   not needed.  The updated one is shorter, which is a plus ;-)

 - The original said "to check out the original branch and stop
   rebasing", and the updated one says "to stop and get back to",
   which is in a more logical order.  

   "the whole rebasing" used as a noun feels something is missing
   there, though.  I wonder if "To get back to the state before you
   started 'rebase -i', run 'git rebase --abort'" is sufficient,
   without saying anything further about abandoning the rebase in
   progress (i.e. "and stop rebasing" or "stop the whole rebasing").

Thanks.

[PATCH] l10n: de.po: fix typo

[Ralf Thielow]

Reported-by: Andre Hinrichs 
Signed-off-by: Ralf Thielow 
---
Hi Andre,

thanks for the bugreport!

Ralf

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

diff --git a/po/de.po b/po/de.po
index 679f8f4..42c4508 100644
--- a/po/de.po
+++ b/po/de.po
@@ -8564,7 +8564,7 @@ msgstr "\"auto-gc\" Modus aktivieren"
 #: builtin/gc.c:362
 msgid "force running gc even if there may be another gc running"
 msgstr ""
-"Ausführung von \"git gc\" erwzingen, selbst wenn ein anderes\n"
+"Ausführung von \"git gc\" erzwingen, selbst wenn ein anderes\n"
 "\"git gc\" bereits ausgeführt wird"
 
 #: builtin/gc.c:379
-- 
2.7.4

Re: [PATCH 1/4] Doc/config.txt: explain repeated sections

[Junio C Hamano]

Jeff King  writes:

> FWIW, the use of "multivalued" here tickled my spider sense, too. I
> think when talking on the list we generally reserve "multivalued" for
> true "we expect this to be a list" variables. But the only mention of
> "multivalued" in the config documentation seems to be:
>
>   Some variables may appear multiple times; we say then that the
>   variable is multivalued.
>
> I think the proposed use is consistent with that (and that line is only
> 2 paragraphs above the proposed paragraph).

Thanks for careful reading.

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

[Junio C Hamano]

Eric Wong  writes:

> To notice similar errors sooner, I wonder if we should have a
> test target for 64-bit users to test with -m32 enabled, somehow.

Thanks.  Hopefully https://travis-ci.org/git/git/jobs/251772744 and
its later incarnations should be sufficient?

Performance improvement for git pull rebase and autostash

[neuling]

Hi,

I have some suggestions to improve performance of 'git pull --rebase'.

1. If I have no new local commits "git pull --rebase" will do a fast 
forward merge. But if I have changes to local files I have to stash them 
also if they are not affected by the new commits from origin. I think in 
that case git should not reject changes to every local file and has to use 
the fast forward merge validation instead.

2. If I have no changes to local files and I use 'git pull --rebase 
--autostash' no stashing should take place.


The improved workflow would look like as follows.


 git pull rebase = true or preserve
   |
   |
   |
   |
   |
check if local branch has no new commits
   |
 no|   yes
  -+- 
  | |
  | |
  | |
   rebase validation  ff merge validation for 
   for local changes  conflicting local changes 
  | |
no changes|  changes no conflicts   |has conflicts 
  +-+--
  ||| |
  ||| |
  ||| |
do rebase use autostash? do ff merge use autostash?
  ||| |
  |no  |  yes   |no   |  yes
  |+-   |-+-
  |||   || |
  |||   || |
  |   conflict error   stash changes|  conflict error stash 
changes
  |||   || |
  |||   || |
  ||do rebase   ||do ff 
merge
  |||   || |
  |||   || |
  ||pop stash   || pop 
stash
  |||   || |
  |||   || |
success  abort   success success   abort success



Regarda,
Mattias

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Jeff King]

On Mon, Jul 10, 2017 at 10:57:57AM -0400, Ben Peart wrote:

> > If this patch can survive a few releases without complaint,
> > then we can feel more confident that designated initializers
> > are widely supported by our user base.  It also is an
> > indication that other C99 features may be supported, but not
> > a guarantee (e.g., gcc had designated initializers before
> > C99 existed).
> 
> Correct.  MSVC also supports designated initializers but does not fully
> support C99.

Out of curiosity, does MSVC define __STDC_VERSION__, and if so, to what?

> > And obviously before releasing this or anything similar, it would be
> > nice to see results from people building pu. I'm especially curious
> > whether MSVC would work with this (or if people even still use it, since
> > Git for Windows is pretty mature?).
> 
> We do use MSVC internally as that gives us access to the great debuggers and
> profilers on the Windows platform.  Fortunately, this particular C99
> construct _is_ supported by MSVC.  I applied the patch below and complied it
> with both MSVC and gcc for Windows and both builds succeeded.

Thanks. This kind of prompt testing and response is very appreciated. It
is unfortunate if we have to pick and choose C99-isms rather than using
the whole thing as a base. But that's probably just reality.

-Peff

Re: [PATCH 0/5] building git with clang/gcc address sanitizer

[Jeff King]

On Mon, Jul 10, 2017 at 04:40:42PM +0200, Lars Schneider wrote:

> > If you want to see it in action, you can do:
> > 
> >  make SANITIZE=address
> >  ./git log -g HEAD HEAD >/dev/null
> > 
> > which finds a bug I recently fixed (but the fix isn't in master yet).
> 
> Do you think it would make sense to run these sanitizers on TravisCI
> to ensure they keep clean? If yes, should we run only "address" or all
> of them (if they run clean)?

Maybe. It's expensive and it's relatively rare that it catches anything.
I used to run valgrind (which is even more expensive) once every release
or so. This is much cheaper, but I've noticed that the Travis
environment is a lot slower than my laptop. So it might take an hour to
run there, which I think would trigger some timeouts?

I guess the best way is to try it and see. I probably wouldn't do an
ASan run for each environment, but just one Linux ASan run, due to the
CPU expense. (TBH, I think the existing gcc versus clang on both
platforms is already slight overkill. But I guess if we have CPU to
burn, more coverage is better than less).

I think "address" is the only one that runs clean right now. With some
work I think we could get "undefined" to run clean. The others, I'm not
so sure.

Some of them can actually be combined in a single build, but I'd have to
dig into the documentation to see which (I think "thread" and "address"
don't work well together, but "undefined" and "address" might). My
SANITIZE trick doesn't handle multiple entries, but it could probably be
taught to.

-Peff

Re: [RFC/PATCH v4 30/49] odb-helper: add read_object_process()

[Ben Peart]

On 6/20/2017 3:55 AM, Christian Couder wrote:

From: Ben Peart 

Signed-off-by: Ben Peart 
Signed-off-by: Christian Couder 
---
  odb-helper.c | 202 ---
  odb-helper.h |   5 ++
  sha1_file.c  |  33 +-
  3 files changed, 227 insertions(+), 13 deletions(-)

diff --git a/odb-helper.c b/odb-helper.c
index 5fb56c6135..20e83cb55a 100644
--- a/odb-helper.c
+++ b/odb-helper.c
@@ -4,6 +4,187 @@
  #include "odb-helper.h"
  #include "run-command.h"
  #include "sha1-lookup.h"
+#include "sub-process.h"
+#include "pkt-line.h"
+#include "sigchain.h"
+
+struct read_object_process {
+   struct subprocess_entry subprocess;
+   unsigned int supported_capabilities;
+};
+
+static int subprocess_map_initialized;
+static struct hashmap subprocess_map;
+
+static void parse_capabilities(char *cap_buf,
+  unsigned int *supported_capabilities,
+  const char *process_name)
+{
+   struct string_list cap_list = STRING_LIST_INIT_NODUP;
+
+   string_list_split_in_place(_list, cap_buf, '=', 1);
+
+   if (cap_list.nr == 2 && !strcmp(cap_list.items[0].string, 
"capability")) {
+   const char *cap_name = cap_list.items[1].string;
+
+   if (!strcmp(cap_name, "get")) {
+   *supported_capabilities |= ODB_HELPER_CAP_GET;
+   } else if (!strcmp(cap_name, "put")) {
+   *supported_capabilities |= ODB_HELPER_CAP_PUT;
+   } else if (!strcmp(cap_name, "have")) {
+   *supported_capabilities |= ODB_HELPER_CAP_HAVE;
+   } else {
+   warning("external process '%s' requested unsupported 
read-object capability '%s'",
+   process_name, cap_name);
+   }
+   }
+
+   string_list_clear(_list, 0);
+}
+
+static int start_read_object_fn(struct subprocess_entry *subprocess)
+{
+   int err;
+   struct read_object_process *entry = (struct read_object_process 
*)subprocess;
+   struct child_process *process = >process;
+   char *cap_buf;
+
+   sigchain_push(SIGPIPE, SIG_IGN);
+
+   err = packet_writel(process->in, "git-read-object-client", "version=1", 
NULL);
+   if (err)
+   goto done;
+
+   err = strcmp(packet_read_line(process->out, NULL), 
"git-read-object-server");
+   if (err) {
+   error("external process '%s' does not support read-object protocol 
version 1", subprocess->cmd);
+   goto done;
+   }
+   err = strcmp(packet_read_line(process->out, NULL), "version=1");
+   if (err)
+   goto done;
+   err = packet_read_line(process->out, NULL) != NULL;
+   if (err)
+   goto done;
+
+   err = packet_writel(process->in, "capability=get", NULL);
+   if (err)
+   goto done;
+
+   while ((cap_buf = packet_read_line(process->out, NULL)))
+   parse_capabilities(cap_buf, >supported_capabilities, 
subprocess->cmd);
+
+done:
+   sigchain_pop(SIGPIPE);
+
+   return err;
+}
+
+static struct read_object_process *launch_read_object_process(const char *cmd)
+{
+   struct read_object_process *entry;
+
+   if (!subprocess_map_initialized) {
+   subprocess_map_initialized = 1;
+   hashmap_init(_map, (hashmap_cmp_fn) cmd2process_cmp, 
0);
+   entry = NULL;
+   } else {
+   entry = (struct read_object_process 
*)subprocess_find_entry(_map, cmd);
+   }
+
+   fflush(NULL);
+
+   if (!entry) {
+   entry = xmalloc(sizeof(*entry));
+   entry->supported_capabilities = 0;
+
+   if (subprocess_start(_map, >subprocess, cmd, 
start_read_object_fn)) {
+   free(entry);
+   return 0;
+   }
+   }
+
+   return entry;
+}
+
+static int check_object_process_error(int err,
+ const char *status,
+ struct read_object_process *entry,
+ const char *cmd,
+ unsigned int capability)
+{
+   if (!err)
+   return;
+
+   if (!strcmp(status, "error")) {
+   /* The process signaled a problem with the file. */
+   } else if (!strcmp(status, "notfound")) {
+   /* Object was not found */
+   err = -1;
+   } else if (!strcmp(status, "abort")) {
+   /*
+* The process signaled a permanent problem. Don't try to read
+* objects with the same command for the lifetime of the current
+* Git process.
+*/
+   if (capability)
+   entry->supported_capabilities &= ~capability;
+   } else {
+

Re: [PATCH 3/4] hook: add signature using "interpret-trailers"

[Ramsay Jones]

On 10/07/17 15:17, Kaartic Sivaraam wrote:
> The sample hook to prepare the commit message before
> a commit allows users to opt-in to add the signature
> to the commit message. The signature is added at a place
> that isn't consistent with the "-s" option of "git commit".
> Further, it could go out of view in certain cases.
> 
> Add the signature in a way similar to "-s" option of
> "git commit" using git's interpret-trailers command.
> 
> It works well in all cases except when the user invokes
> "git commit" without any arguments. In that case manually
> add a new line after the first line to ensure it's consistent
> with the output of "-s" option.
> 

Again, s/signature/sign-off/g, or similar (including subject line).

ATB,
Ramsay Jones


> Signed-off-by: Kaartic Sivaraam 
> ---
>  templates/hooks--prepare-commit-msg.sample | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/templates/hooks--prepare-commit-msg.sample 
> b/templates/hooks--prepare-commit-msg.sample
> index 708f0e92c..a15d6d634 100755
> --- a/templates/hooks--prepare-commit-msg.sample
> +++ b/templates/hooks--prepare-commit-msg.sample
> @@ -32,4 +32,8 @@ SHA1=$3
>  # esac
>  
>  # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: 
> \1/p')
> -# grep -qs "^$SOB" "$COMMIT_MSG_FILE" || echo "$SOB" >> "$COMMIT_MSG_FILE"
> +# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
> +# if test -z "$COMMIT_SOURCE"
> +# then
> +#   @PERL_PATH@ -i.bak -pe 'print "\n" if($first_line++ == 0)' 
> "$COMMIT_MSG_FILE"
> +# fi
> 

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Ben Peart]

On 7/10/2017 3:03 AM, Jeff King wrote:

On Sun, Jul 09, 2017 at 10:05:49AM -0700, Junio C Hamano wrote:


René Scharfe  writes:


I wonder when we can begin to target C99 in git's source, though. :)


Let's get the ball rolling by starting to use some of the useful
features like designated initializers, perhaps, in a small, critical
and reasonably stable part of the system that anybody must compile,
leave it in one full release cycle or two, and when we hear nobody
complains, introduce it en masse for the remainder of the system?

That way, we will see if there are people who need pre-C99 soon
enough, and we won't have to scramble reverting too many changes
when it happens.


Neat idea. Something like this?

-- >8 --
Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT

There are certain C99 features that might be nice to use in
our code base, but we've hesitated to do so in order to
avoid breaking compatibility with older compilers. But we
don't actually know if people are even using pre-C99
compilers these days.

One way to figure that out is to introduce a very small use
of a feature, and see if anybody complains. The strbuf code
is a good place to do this for a few reasons:

   - it always gets compiled, no matter which Makefile knobs
 have been tweaked.

   - it's very stable; this definition hasn't changed in a
 long time and is not likely to (so if we have to revert,
 it's unlikely to cause headaches)

If this patch can survive a few releases without complaint,
then we can feel more confident that designated initializers
are widely supported by our user base.  It also is an
indication that other C99 features may be supported, but not
a guarantee (e.g., gcc had designated initializers before
C99 existed).



Correct.  MSVC also supports designated initializers but does not fully 
support C99.



And if we do get complaints, then we'll have gained some
data and we can easily revert this patch.

Signed-off-by: Jeff King 
---
I suspected we could also do something with __STDC_VERSION__, though I
wonder what compilers set it to when not in standards-compliant mode.
gcc-6 claims C11 when no specific -std flag is given.

And obviously before releasing this or anything similar, it would be
nice to see results from people building pu. I'm especially curious
whether MSVC would work with this (or if people even still use it, since
Git for Windows is pretty mature?).



We do use MSVC internally as that gives us access to the great debuggers 
and profilers on the Windows platform.  Fortunately, this particular C99 
construct _is_ supported by MSVC.  I applied the patch below and 
complied it with both MSVC and gcc for Windows and both builds succeeded.



  strbuf.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index 2075384e0..e705b94db 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -68,7 +68,7 @@ struct strbuf {
  };
  
  extern char strbuf_slopbuf[];

-#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
+#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
  
  /**

   * Life Cycle Functions

Re: [PATCH 0/5] building git with clang/gcc address sanitizer

[Lars Schneider]

> On 10 Jul 2017, at 15:24, Jeff King  wrote:
> 
> I mentioned recently that I sometimes run the test suite with ASan, so
> here are a few tweaks to make this easier (most of which I've been
> carrying in my personal config.mak for a few years).
> 
> In my experience ASan does at least as well as valgrind at finding
> memory bugs, and runs way faster. With this series, running:
> 
>  make SANITIZE=address test
> 
> takes about 4.5 minutes on my machine. A normal build+test is about 1.5
> minutes on the same machine. I haven't timed a full run with --valgrind
> recently, but I know that it is much much slower. :)
> 
> If you want to see it in action, you can do:
> 
>  make SANITIZE=address
>  ./git log -g HEAD HEAD >/dev/null
> 
> which finds a bug I recently fixed (but the fix isn't in master yet).

Do you think it would make sense to run these sanitizers on TravisCI
to ensure they keep clean? If yes, should we run only "address" or all
of them (if they run clean)?

- Lars


> 
> There are other sanitizers, too. I _thought_ I had
> 
>  make SANITIZE=undefined test
> 
> running cleanly at one point, but it's definitely not clean now. Patch 5
> helps a little by disabling unaligned loads in our get_be32() macros.
> Once upon a time I had to set INTERNAL_QSORT, but it isn't necessary
> anymore since everything goes through sane_qsort(). Most of the
> remaining bugs are of a similar form, doing something like:
> 
>  memcpy(NULL, NULL, 0);
> 
> I don't know if it's worth having a sane_memcpy() there, or simply
> tweaking the code to avoid the call (almost all of them are a single
> call from apply.c:2870).
> 
> It looks like we also have a case of shifting off the left side of a
> signed int, which is undefined.
> 
> You can also try:
> 
>  make SANITIZE=thread test
> 
> but it's not clean. I poked at some of the errors, and I don't think
> there a problem in practice (though they may be worth cleaning up in the
> name of code hygiene).
> 
> There's also:
> 
>  make SANITIZE=memory test
> 
> This is clang-only. It's supposed to find uses of uninitialized memory.
> But it complains about writing out anything that zlib has touched. I
> seem to recall that valgrind had a similar problem. I'm not sure what
> zlib does to confuse these analyzers. For valgrind we were able to add a
> suppression. We could probably do the same here, but I haven't looked
> into it.
> 
>  [1/5]: test-lib: set ASAN_OPTIONS variable before we run git
>  [2/5]: test-lib: turn on ASan abort_on_error by default
>  [3/5]: Makefile: add helper for compiling with -fsanitize
>  [4/5]: Makefile: turn off -fomit-frame-pointer with sanitizers
>  [5/5]: Makefile: disable unaligned loads with UBSan
> 
> Makefile  |  8 
> t/test-lib.sh | 11 ---
> 2 files changed, 16 insertions(+), 3 deletions(-)
> 
> -Peff

Re: [PATCH v5 0/7] Fast git status via a file system watcher

[Ben Peart]

On 7/10/2017 9:36 AM, Ben Peart wrote:



On 6/28/2017 1:11 AM, Christian Couder wrote:

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

Changes from V4 include:

...

I took a look at this patch series except the last patch ([PATCH v5
7/7] fsmonitor: add a performance test) as Junio reviewed it already,
and had only a few comments on patches 3/7 and 4/7.

I am still not convinced by the discussions following v2
(http://public-inbox.org/git/20170518201333.13088-1-benpe...@microsoft.com/) 


about using a hook instead of for example a "core.fsmonitorcommand".

I think using a hook is not necessary and might not be a good match
for later optimizations. For example people might want to use a
library or some OS specific system calls to do what the hook does.

AEvar previously reported some not so great performance numbers on
some big Booking.com boxes with a big monorepo and it seems that using
taskset for example to make sure that the hook is run on the same CPU
improves these numbers significantly. So avoiding to run a separate
process can be important in some cases.



Using a hook is the only pattern I've seen in git that provides a way to 
enable OS specific calls.  I used a hook so that different file 
monitoring services could be plugged in depending on the OS or tools 
available.  The Watchman integration script was mostly intended as a 
sample that could be used where Watchman is available and works well.


I had not heard about the taskset issues you mention above.  If there is 
something else that can be done to make this work better, please let me 
know the details.


If I'm understanding you correctly, you are suggesting that someone 
should be able to configure a setting (core.fsmonitorcommand) that gives 
a custom command line that would be run instead of running the 
query-fsmonitor hook.


I'm not entirely sure how that should work.  There are command line 
options that need to be passed (currently the interface version as well 
as the current clock in nanoseconds).  How would those passed when using 
the custom command?


Is it OK to just append them to the given command line?  Does there need 
to be some substitution token to indicate where they should be inserted 
(ie "mycustomcommand --custom --options %version% --more-options 
%timestamp%").  Are there any other tokens that should be supported (ie 
PID or processor mask?).


Is there a design pattern already used somewhere in git that I can 
follow or is this all blazing a new trail?




My co-worker reminded me about git difftool - is this what you had in mind?


Thanks for continuing to look into this. Feedback is good!

[PATCH] doc: correct a mistake in an illustration

[Kaartic Sivaraam]

The first illustration of the "RECOVERING FROM UPSTREAM REBASE"
section in the 'git-rebase' documentation wasn't in line with the
rest of the illustrations of that section.

Correct it.

Signed-off-by: Kaartic Sivaraam 
---
 Documentation/git-rebase.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 53f4e1444..652a99062 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -675,7 +675,7 @@ on this 'subsystem'.  You might end up with a history like 
the
 following:
 
 
-o---o---o---o---o---o---o---o---o  master
+o---o---o---o---o---o---o---o  master
 \
  o---o---o---o---o  subsystem
   \
-- 
2.13.2.957.g457671ade

[PATCH 3/4] hook: add signature using "interpret-trailers"

[Kaartic Sivaraam]

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

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

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

Signed-off-by: Kaartic Sivaraam 
---
 templates/hooks--prepare-commit-msg.sample | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/templates/hooks--prepare-commit-msg.sample 
b/templates/hooks--prepare-commit-msg.sample
index 708f0e92c..a15d6d634 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -32,4 +32,8 @@ SHA1=$3
 # esac
 
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
-# grep -qs "^$SOB" "$COMMIT_MSG_FILE" || echo "$SOB" >> "$COMMIT_MSG_FILE"
+# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
+# if test -z "$COMMIT_SOURCE"
+# then
+#   @PERL_PATH@ -i.bak -pe 'print "\n" if($first_line++ == 0)' 
"$COMMIT_MSG_FILE"
+# fi
-- 
2.13.2.957.g457671ade

[PATCH 4/4] hook: add a simple first example

[Kaartic Sivaraam]

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

Also, update the documentation.

Signed-off-by: Kaartic Sivaraam 
---
 I made an attempt to make the second example work with amending 
 with the aim of making it suitable for usage out of the box. It
 seems that it's not easy to make it work as the status of a file
 cannot be determined correctly when the index while amending
 introduces changes to a file that has a change in the commit being
 amended.

 Is there any way in which the second example could be made to work with
 amending without much effort? I'm asking this assuming something might
 have happened, since the script was added, that could ease the task.

 Documentation/githooks.txt | 3 +++
 templates/hooks--prepare-commit-msg.sample | 5 -
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index fdc01aa25..59f38efba 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -121,6 +121,9 @@ it is not suppressed by the `--no-verify` option.  A 
non-zero exit
 means a failure of the hook and aborts the commit.  It should not
 be used as replacement for pre-commit hook.
 
+The sample `prepare-commit-msg` hook that comes with Git removes the
+help message found in the commented portion of the commit template.
+
 commit-msg
 ~~
 
diff --git a/templates/hooks--prepare-commit-msg.sample 
b/templates/hooks--prepare-commit-msg.sample
index a15d6d634..a84c3e5a8 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -9,7 +9,8 @@
 #
 # To enable this hook, rename this file to "prepare-commit-msg".
 
-# This hook includes three examples.
+# This hook includes three examples.  The first one removes the
+# "# Please enter the commit message..." help message.
 #
 # The second includes the output of "git diff --name-status -r"
 # into the message, just before the "git status" output.  It is
@@ -23,6 +24,8 @@ COMMIT_MSG_FILE=$1
 COMMIT_SOURCE=$2
 SHA1=$3
 
+@PERL_PATH@ -i.bak -ne 'print unless(m/^. Please enter the commit 
message/..m/^#$/)' "$COMMIT_MSG_FILE"
+
 # case "$COMMIT_SOURCE,$SHA1" in
 #  ,|template,)
 #@PERL_PATH@ -i.bak -pe '
-- 
2.13.2.957.g457671ade

[PATCH 2/4] hook: name the positional variables

[Kaartic Sivaraam]

It's always nice to have named variables instead of
positional variables as they communicate their purpose
well.

Appropriately name the positional variables of the hook
to make it easier to see what's going on.

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

diff --git a/templates/hooks--prepare-commit-msg.sample 
b/templates/hooks--prepare-commit-msg.sample
index b8ba335cf..708f0e92c 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -19,14 +19,17 @@
 # The third example adds a Signed-off-by line to the message, that can
 # still be edited.  This is rarely a good idea.
 
+COMMIT_MSG_FILE=$1
+COMMIT_SOURCE=$2
+SHA1=$3
 
-# case "$2,$3" in
+# case "$COMMIT_SOURCE,$SHA1" in
 #  ,|template,)
 #@PERL_PATH@ -i.bak -pe '
 #   print "\n" . `git diff --cached --name-status -r`
-#   if /^#/ && $first++ == 0' "$1" ;;
+#   if /^#/ && $first++ == 0' "$COMMIT_MSG_FILE" ;;
 #  *) ;;
 # esac
 
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
-# grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
+# grep -qs "^$SOB" "$COMMIT_MSG_FILE" || echo "$SOB" >> "$COMMIT_MSG_FILE"
-- 
2.13.2.957.g457671ade

[PATCH 1/4] hook: cleanup script

[Kaartic Sivaraam]

Prepare the 'preare-commit-msg' sample script for
upcoming changes. Preparation includes removal of
an example that has outlived it's purpose. The example
is the one that comments the "Conflicts:" part of a
merge commit message. It isn't relevant anymore as
it's done by default since 261f315b ("merge & sequencer:
turn "Conflicts:" hint into a comment", 2014-08-28).

Further remove the relevant comments from the sample script
and update the documentation.

Signed-off-by: Kaartic Sivaraam 
---
 Documentation/githooks.txt |  3 ---
 templates/hooks--prepare-commit-msg.sample | 20 
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 706091a56..fdc01aa25 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -121,9 +121,6 @@ it is not suppressed by the `--no-verify` option.  A 
non-zero exit
 means a failure of the hook and aborts the commit.  It should not
 be used as replacement for pre-commit hook.
 
-The sample `prepare-commit-msg` hook that comes with Git comments
-out the `Conflicts:` part of a merge's commit message.
-
 commit-msg
 ~~
 
diff --git a/templates/hooks--prepare-commit-msg.sample 
b/templates/hooks--prepare-commit-msg.sample
index 86b8f227e..b8ba335cf 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -9,8 +9,7 @@
 #
 # To enable this hook, rename this file to "prepare-commit-msg".
 
-# This hook includes three examples.  The first comments out the
-# "Conflicts:" part of a merge commit.
+# This hook includes three examples.
 #
 # The second includes the output of "git diff --name-status -r"
 # into the message, just before the "git status" output.  It is
@@ -20,17 +19,14 @@
 # The third example adds a Signed-off-by line to the message, that can
 # still be edited.  This is rarely a good idea.
 
-case "$2,$3" in
-  merge,)
-@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' 
"$1" ;;
 
-# ,|template,)
-#   @PERL_PATH@ -i.bak -pe '
-#  print "\n" . `git diff --cached --name-status -r`
-#   if /^#/ && $first++ == 0' "$1" ;;
-
-  *) ;;
-esac
+# case "$2,$3" in
+#  ,|template,)
+#@PERL_PATH@ -i.bak -pe '
+#   print "\n" . `git diff --cached --name-status -r`
+#   if /^#/ && $first++ == 0' "$1" ;;
+#  *) ;;
+# esac
 
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
 # grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
-- 
2.13.2.957.g457671ade

Re: "Branch exists" error while trying to rename a non-existing branch to an existing one

[Kaartic Sivaraam]

On Sun, 2017-07-09 at 11:57 -0700, Junio C Hamano wrote:
> This is borderline "meh" at least to me.  An argument against a
> hypothetical version of Git that "fixes" your issue would be that no
> matter what the source of renaming is, as long as 'master' exists,
> "branch -m" shouldn't overwrite it, and it is a good thing to remind
> the user that 'master' exists and the user meant to rename it to
> something else.
> 
I'm not against the fact of reminding the user about an existing
branch. I'm with the fact that, warn him when he really has to care
about a branch being overwritten i.e., when he tries to rename an
"existing" branch to one that refers to another existing branch.

I found this behaviour odd as I try to relate it with the 'mv' command.
It's behaviour is as follows,

$ ls
file  some_file
$ mv nothing file
mv: cannot stat 'nothing': No such file or directory

If I haven't missed anything the following patch seems to fix the
problem,

diff --git a/builtin/branch.c b/builtin/branch.c
index 48a513a84..2869aaca8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -476,7 +476,10 @@ static void rename_branch(const char *oldname, const char 
*newname, int force)
 */
clobber_head_ok = !strcmp(oldname, newname);
 
-   validate_new_branchname(newname, , force, clobber_head_ok);
+   if(ref_exists(oldref.buf))
+   validate_new_branchname(newname, , force, 
clobber_head_ok);
+   else
+   die(_("Branch '%s' does not exist."), oldname);
 
reject_rebase_or_bisect_branch(oldref.buf);


-- 
Kaartic

Re: [PATCH v5 0/7] Fast git status via a file system watcher

[Ben Peart]

On 6/28/2017 1:11 AM, Christian Couder wrote:

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

Changes from V4 include:

...

I took a look at this patch series except the last patch ([PATCH v5
7/7] fsmonitor: add a performance test) as Junio reviewed it already,
and had only a few comments on patches 3/7 and 4/7.

I am still not convinced by the discussions following v2
(http://public-inbox.org/git/20170518201333.13088-1-benpe...@microsoft.com/)
about using a hook instead of for example a "core.fsmonitorcommand".

I think using a hook is not necessary and might not be a good match
for later optimizations. For example people might want to use a
library or some OS specific system calls to do what the hook does.

AEvar previously reported some not so great performance numbers on
some big Booking.com boxes with a big monorepo and it seems that using
taskset for example to make sure that the hook is run on the same CPU
improves these numbers significantly. So avoiding to run a separate
process can be important in some cases.



Using a hook is the only pattern I've seen in git that provides a way to 
enable OS specific calls.  I used a hook so that different file 
monitoring services could be plugged in depending on the OS or tools 
available.  The Watchman integration script was mostly intended as a 
sample that could be used where Watchman is available and works well.


I had not heard about the taskset issues you mention above.  If there is 
something else that can be done to make this work better, please let me 
know the details.


If I'm understanding you correctly, you are suggesting that someone 
should be able to configure a setting (core.fsmonitorcommand) that gives 
a custom command line that would be run instead of running the 
query-fsmonitor hook.


I'm not entirely sure how that should work.  There are command line 
options that need to be passed (currently the interface version as well 
as the current clock in nanoseconds).  How would those passed when using 
the custom command?


Is it OK to just append them to the given command line?  Does there need 
to be some substitution token to indicate where they should be inserted 
(ie "mycustomcommand --custom --options %version% --more-options 
%timestamp%").  Are there any other tokens that should be supported (ie 
PID or processor mask?).


Is there a design pattern already used somewhere in git that I can 
follow or is this all blazing a new trail?


Thanks for continuing to look into this. Feedback is good!

Re: [PATCH 0/5] building git with clang/gcc address sanitizer

[Jeff King]

On Mon, Jul 10, 2017 at 09:24:18AM -0400, Jeff King wrote:

> You can also try:
> 
>   make SANITIZE=thread test
> 
> but it's not clean. I poked at some of the errors, and I don't think
> there a problem in practice (though they may be worth cleaning up in the
> name of code hygiene).

Here's an example that I fixed up long ago, but never quite had the
stomach to seriously propose.

If it were the last one, it might be worth applying to get a clean run
of "make test", but I think it's just one of many.

-- >8 --
Date: Mon, 8 Dec 2014 03:02:34 -0500
Subject: [PATCH] transport-helper: initialize debug flag before starting
 threads

The transport_debug() function uses a static variable to
lazily cache the boolean value of $TRANSPORT_DEBUG. If a
program uses transport-helper's bidirectional_transfer_loop
(as remote-ext and remote-fd do), then two threads may both
try to write the variable at the same time.

We can fix this by initializing the variable right before
starting the threads.

Noticed by "-fsanitize=thread". This probably isn't a
problem in practice, as both threads will write the same
value, and we are unlikely to see a torn write on an "int"
(i.e., on sane platforms a write to an int is atomic).

Signed-off-by: Jeff King 
---
 transport-helper.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 33cff38cc..76f19ddb0 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1113,17 +1113,23 @@ int transport_helper_init(struct transport *transport, 
const char *name)
 /* This should be enough to hold debugging message. */
 #define PBUFFERSIZE 8192
 
+static int transport_debug_enabled(void)
+{
+   static int debug_enabled = -1;
+
+   if (debug_enabled < 0)
+   debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
+   return debug_enabled;
+}
+
 /* Print bidirectional transfer loop debug message. */
 __attribute__((format (printf, 1, 2)))
 static void transfer_debug(const char *fmt, ...)
 {
va_list args;
char msgbuf[PBUFFERSIZE];
-   static int debug_enabled = -1;
 
-   if (debug_enabled < 0)
-   debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
-   if (!debug_enabled)
+   if (!transport_debug_enabled())
return;
 
va_start(args, fmt);
@@ -1292,6 +1298,10 @@ static int tloop_spawnwait_tasks(struct 
bidirectional_transfer_state *s)
pthread_t ptg_thread;
int err;
int ret = 0;
+
+   /* initialize static global debug flag as a side effect */
+   transport_debug_enabled();
+
err = pthread_create(_thread, NULL, udt_copy_task_routine,
>gtp);
if (err)
-- 
2.13.2.1071.gcd8104b61

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

[Jeff King]

On Sun, Jul 09, 2017 at 09:59:33AM -0700, Junio C Hamano wrote:

> >> After step 6/7, we no longer "allow cycles in reflog ancestry", as
> >> there will be no reflog ancestry to speak of ;-), so it would be
> >> nice to remove the comment above in that step.  But alternatively,
> >> we can rephrase the comment here, to say something like "the same
> >> commit can be shown multiple times while showing entries from the
> >> reflog" instead.
> >
> > I actually think the comment is a bit obtuse in the first place. The
> > real issue is that we show commits multiple times. That's caused by
> > cycles, yes, but also by us clearing the SEEN flag. ;)
> >
> > Maybe this on top?
> 
> Yup, that is a much better version of what I had in mind that can go
> either before this step as a preparatory cleanup, squashed into this
> as "while at it", or after the series as a finishing touches.  The
> last one will let the codebase lie for a short while, though, so I am
> likely to squash it in or wiggle it under.

Where you put it on jk/reflog-walk in your tree is fine, though note the
commit message is slightly inaccurate there (it says "commit buffer and
parents" but at that point it is really just "commit buffer").

-Peff

[PATCH 4/5] Makefile: turn off -fomit-frame-pointer with sanitizers

[Jeff King]

The ASan manual recommends disabling this optimization, as
it can make the backtraces produced by the tool harder to
follow (and since this is a test-debug build, we don't care
about squeezing out every last drop of performance).

Signed-off-by: Jeff King 
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 59f6bdcd7..cc03b3c95 100644
--- a/Makefile
+++ b/Makefile
@@ -1014,6 +1014,7 @@ endif
 
 ifdef SANITIZE
 BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
+BASIC_CFLAGS += -fno-omit-frame-pointer
 endif
 
 ifndef sysconfdir
-- 
2.13.2.1071.gcd8104b61

[PATCH 5/5] Makefile: disable unaligned loads with UBSan

[Jeff King]

The undefined behavior sanitizer complains about unaligned
loads, even if they're OK for a particular platform in
practice. It's possible that they _are_ a problem, of
course, but since it's a known tradeoff the UBSan errors are
just noise.

Let's quiet it automatically by building with
NO_UNALIGNED_LOADS when SANITIZE=undefined is in use.

Signed-off-by: Jeff King 
---
 Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Makefile b/Makefile
index cc03b3c95..3c341b2a6 100644
--- a/Makefile
+++ b/Makefile
@@ -1015,6 +1015,9 @@ endif
 ifdef SANITIZE
 BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
 BASIC_CFLAGS += -fno-omit-frame-pointer
+ifeq ($(SANITIZE),undefined)
+BASIC_CFLAGS += -DNO_UNALIGNED_LOADS
+endif
 endif
 
 ifndef sysconfdir
-- 
2.13.2.1071.gcd8104b61

[PATCH 3/5] Makefile: add helper for compiling with -fsanitize

[Jeff King]

You can already build and test with ASan by doing:

  make CFLAGS=-fsanitize=address test

but there are a few slight annoyances:

  1. It's a little long to type.

  2. It override your CFLAGS completely. You'd probably
 still want -O2, for instance.

  3. It's a good idea to also turn off "recovery", which
 lets the program keep running after a problem is
 detected (with the intention of finding as many bugs as
 possible in a given run). Since Git's test suite should
 generally run without triggering any problems, it's
 better to abort immediately and fail the test when we
 do find an issue.

With this patch, all of that happens automatically when you
run:

  make SANITIZE=address test

Signed-off-by: Jeff King 
---
 Makefile | 4 
 1 file changed, 4 insertions(+)

diff --git a/Makefile b/Makefile
index 9c9c42f8f..59f6bdcd7 100644
--- a/Makefile
+++ b/Makefile
@@ -1012,6 +1012,10 @@ ifdef DEVELOPER
 CFLAGS += $(DEVELOPER_CFLAGS)
 endif
 
+ifdef SANITIZE
+BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)
+endif
+
 ifndef sysconfdir
 ifeq ($(prefix),/usr)
 sysconfdir = /etc
-- 
2.13.2.1071.gcd8104b61

[PATCH 2/5] test-lib: turn on ASan abort_on_error by default

[Jeff King]

By default, ASan will exit with code 1 when it sees an
error. This means we'll notice a problem when we expected
git to succeed, but not in a test_must_fail block.

Let's ask it to actually raise SIGABRT instead. That will
give us a signal death that test_must_fail will notice. As a
bonus, it may also leave a coredump, which can be handy for
digging into a failure.

Signed-off-by: Jeff King 
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 961194a50..1b6e53f78 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -41,7 +41,7 @@ GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 # the noise level. This needs to happen at the start of the script,
 # before we even do our "did we build git yet" check (since we don't
 # want that one to complain to stderr).
-: ${ASAN_OPTIONS=detect_leaks=0}
+: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
 export ASAN_OPTIONS
 
 
-- 
2.13.2.1071.gcd8104b61

[PATCH 0/5] building git with clang/gcc address sanitizer

[Jeff King]

I mentioned recently that I sometimes run the test suite with ASan, so
here are a few tweaks to make this easier (most of which I've been
carrying in my personal config.mak for a few years).

In my experience ASan does at least as well as valgrind at finding
memory bugs, and runs way faster. With this series, running:

  make SANITIZE=address test

takes about 4.5 minutes on my machine. A normal build+test is about 1.5
minutes on the same machine. I haven't timed a full run with --valgrind
recently, but I know that it is much much slower. :)

If you want to see it in action, you can do:

  make SANITIZE=address
  ./git log -g HEAD HEAD >/dev/null

which finds a bug I recently fixed (but the fix isn't in master yet).

There are other sanitizers, too. I _thought_ I had

  make SANITIZE=undefined test

running cleanly at one point, but it's definitely not clean now. Patch 5
helps a little by disabling unaligned loads in our get_be32() macros.
Once upon a time I had to set INTERNAL_QSORT, but it isn't necessary
anymore since everything goes through sane_qsort(). Most of the
remaining bugs are of a similar form, doing something like:

  memcpy(NULL, NULL, 0);

I don't know if it's worth having a sane_memcpy() there, or simply
tweaking the code to avoid the call (almost all of them are a single
call from apply.c:2870).

It looks like we also have a case of shifting off the left side of a
signed int, which is undefined.

You can also try:

  make SANITIZE=thread test

but it's not clean. I poked at some of the errors, and I don't think
there a problem in practice (though they may be worth cleaning up in the
name of code hygiene).

There's also:

  make SANITIZE=memory test

This is clang-only. It's supposed to find uses of uninitialized memory.
But it complains about writing out anything that zlib has touched. I
seem to recall that valgrind had a similar problem. I'm not sure what
zlib does to confuse these analyzers. For valgrind we were able to add a
suppression. We could probably do the same here, but I haven't looked
into it.

  [1/5]: test-lib: set ASAN_OPTIONS variable before we run git
  [2/5]: test-lib: turn on ASan abort_on_error by default
  [3/5]: Makefile: add helper for compiling with -fsanitize
  [4/5]: Makefile: turn off -fomit-frame-pointer with sanitizers
  [5/5]: Makefile: disable unaligned loads with UBSan

 Makefile  |  8 
 t/test-lib.sh | 11 ---
 2 files changed, 16 insertions(+), 3 deletions(-)

-Peff

[PATCH 1/5] test-lib: set ASAN_OPTIONS variable before we run git

[Jeff King]

We turn off ASan's leak detection by default in the test
suite because it's too noisy. But we don't do so until
part-way through test-lib. This is before we've run any
tests, but after we do our initial "./git" to see if the
binary has even been built.

When built with clang, this seems to work fine. However,
using "gcc -fsanitize=address", the leak checker seems to
complain more aggressively:

  $ ./git
  ...
  ==5352==ERROR: LeakSanitizer: detected memory leaks
  Direct leak of 2 byte(s) in 1 object(s) allocated from:
  #0 0x7f120e7afcf8 in malloc 
(/usr/lib/x86_64-linux-gnu/libasan.so.3+0xc1cf8)
  #1 0x559fc2a3ce41 in do_xmalloc /home/peff/compile/git/wrapper.c:60
  #2 0x559fc2a3cf1a in do_xmallocz /home/peff/compile/git/wrapper.c:100
  #3 0x559fc2a3d0ad in xmallocz /home/peff/compile/git/wrapper.c:108
  #4 0x559fc2a3d0ad in xmemdupz /home/peff/compile/git/wrapper.c:124
  #5 0x559fc2a3d0ad in xstrndup /home/peff/compile/git/wrapper.c:130
  #6 0x559fc274535a in main /home/peff/compile/git/common-main.c:39
  #7 0x7f120dabd2b0 in __libc_start_main 
(/lib/x86_64-linux-gnu/libc.so.6+0x202b0)

This is a leak in the sense that we never free it, but it's
in a global that is meant to last the whole program. So it's
not really interesting or in need of fixing. And at any
rate, mentioning leaks outside of the test_expect blocks is
certainly unwelcome, as it pollutes stderr.

Let's bump the setting of ASAN_OPTIONS higher in test-lib.sh
to catch our initial "can we even run git?" test.  While
we're at it, we can add a comment to make it a bit less
inscrutable.

Signed-off-by: Jeff King 
---
 t/test-lib.sh | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 2306574dc..961194a50 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -36,6 +36,14 @@ then
 fi
 GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 
+# If we were built with ASAN, it may complain about leaks
+# of program-lifetime variables. Disable it by default to lower
+# the noise level. This needs to happen at the start of the script,
+# before we even do our "did we build git yet" check (since we don't
+# want that one to complain to stderr).
+: ${ASAN_OPTIONS=detect_leaks=0}
+export ASAN_OPTIONS
+
 
 # It appears that people try to run tests without building...
 "$GIT_BUILD_DIR/git" >/dev/null
@@ -148,9 +156,6 @@ else
}
 fi
 
-: ${ASAN_OPTIONS=detect_leaks=0}
-export ASAN_OPTIONS
-
 # Protect ourselves from common misconfiguration to export
 # CDPATH into the environment
 unset CDPATH
-- 
2.13.2.1071.gcd8104b61

Darlehensangebot gelten jetzt

[Ocean Finance®]

Schönen Tag,

  Brauchen Sie eine finanzielle Unterstützung? Brauchen Sie einen legitimen 
Kredit für Zinsen? Brauchen Sie ein Business-Darlehen? Brauchen Sie ein 
Darlehen, um ein Haus zu kaufen, Auto, zahlen Sie Ihre Rechnungen und Schulden? 
Brauchen Sie Geld, um Probleme zu lösen? Wenn so freundlich für ein Darlehen 
mit uns um 3% Zinssatz beantragen, bewerben Sie sich jetzt durch die folgenden 
Details und zurück zu uns per E-Mail: oceanfinanceloanuk...@gmail.com

INFORMATIONEN BENÖTIGT

Deine Namen:

Adresse: ...
Telefon: ...
Benötigte Menge: ...
Dauer: ...
Beruf: ...
Monatliches Einkommensniveau: ..
Geschlecht: ..
Geburtsdatum: ...
Bundesland: ...
Land: .
Zweck: .

Danke für dein Verständnis

Dringende Antwort von Ihnen jetzt benötigt

Mit freundlichen Grüßen

Robert Wade

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

[Jeff King]

On Mon, Jul 10, 2017 at 09:42:55AM +, Eric Wong wrote:

> > Thanks, I was able to reproduce with CFLAGS=-m32.
> 
> No problem and thanks for the quick response!  Latest pu is
> fine for me.
> 
> To notice similar errors sooner, I wonder if we should have a
> test target for 64-bit users to test with -m32 enabled, somehow.

It's "just"

  make CFLAGS=-m32 test

once you are setup for building 32-bit binaries. But that setup can be a
bit tricky.  On Debian, I have multi-arch set up, and then I have to
"apt-get install zlib1g-dev:i386" etc for all the dependencies. Which
isn't _too_ bad except that libcurl4-openssl-dev conflicts between the
amd64 and i386 versions due to /usr/bin/curl-config. So I have to
install the i386 package, do the -m32 build, and then reinstall the
amd64 one to go back to my regular builds.

At any rate, the 32-bit thing in this instance[1] mostly just tickled
the bug in a different way. Testing with either ASan or valgrind also
detected the problem on 64-bit machines, and people (including me) do
run those periodically. In this case the bug was years old. The reason
it wasn't caught earlier is that there wasn't test coverage, and my
series added it for other reasons. So you really _did_ catch it as soon
as it hit pu, which seems pretty reasonable.

-Peff

[1] There are bugs that really are 32-bit-only, of course, and
instrumenting tools wouldn't catch those. I think we had a bug a
while back where the code mistook sizeof(uint64_t*) for
sizeof(uint64_t). They're the same on 64-bit platforms, but not on
32-bit ones. I do think such bugs are relatively rare, though.

Re: Small typo in german translation

[Kevin Daudt]

On Mon, Jul 10, 2017 at 08:47:23AM +0200, Andre Hinrichs wrote:
> Hi Git-Developers!
> 
> I've found a small typo in git/po/de.po
> In line 8567 the word "erwzingen" should be "erzwingen".
> Please fix.
> 
> Thanks
> 

Hello Andre,

The best way to get this fixed is to make a pull-request on the German
translation team repo[0].

Kind regards, Kevin

[0]: https://github.com/ralfth/git-po-de