Re: [PATCH 2/2] fsmonitor: Store fsmonitor bitmap before splitting index

[Junio C Hamano]

Alex Vandiver  writes:

> ba1b9caca6 resolved the problem of the fsmonitor data being applied to

(from SubmittingPatches)

If you want to reference a previous commit in the history of a stable
branch, use the format "abbreviated sha1 (subject, date)",
with the subject enclosed in a pair of double-quotes, like this:

Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30)
noticed that ...

The "Copy commit summary" command of gitk can be used to obtain this
format, or this invocation of "git show":

git show -s --date=short --pretty='format:%h ("%s", %ad)' 


> the non-base index when reading; however, a similar problem exists
> when writing the index.  Specifically, writing of the fsmonitor
> extension happens only after the work to split the index has been
> applied -- as such, the information in the index is only for the
> non-"base" index, and thus the extension information contains only
> partial data.

So... what's the effect of not applying this change?  Do we miss
paths that are known to the watchman to have been modified and end
up not adding them if we do "git add -u"?  Or do we miss paths that
are known to the watchman to be clean but mistakenly think are dirty,
and spend unnecessary cycles?  IOW, is this fixing a correctness
issue, or a performance one?

> When saving, compute the ewah bitmap before the index is split, and
> store it in the fsmonitor_dirty field, mirroring the behavior that
> occurred during reading.  fsmonitor_dirty is kept from being leaked by
> being freed when the extension data is written -- which always happens
> precisely once, no matter the split index configuration.

The observation and the approach stated to fix both sounds
sensible.  I'll queue this too, awaiting for Ben's review.

Thanks.

Re: [PATCH 1/2] fsmonitor: Read from getcwd(), not the PWD environment variable

[Junio C Hamano]

Alex Vandiver  writes:

This comment is only so that I do not keep editing them while queuing..

> Subject: Re: [PATCH 1/2] fsmonitor: Read from getcwd(), not the PWD 
> environment variable

Downcase "Read" (or any word after ": " on the commit title).

> Though the process has chdir'd to the root of the working tree, the
> PWD environment variable is only guaranteed to be updated accordingly
> if a shell is involved -- which is not guaranteed to be the case.
> That is, if `/usr/bin/perl` is a binary, $ENV{PWD} is unchanged from
> whatever spawned `git` -- if `/usr/bin/perl` is a trivial shell
> wrapper to the real `perl`, `$ENV{PWD}` will have been updated to the
> root of the working copy.
>
> Update to read from the Cwd module using the `getcwd` syscall, not the
> PWD environment variable.  The Cygwin case is left unchanged, as it
> necessarily _does_ go through a shell.

Interesting observation.  Why didn't anybody else notice it, I
wonder?

Thanks, will queue.

> Signed-off-by: Alex Vandiver 
> ---
>  t/t7519/fsmonitor-watchman | 3 ++-
>  templates/hooks--fsmonitor-watchman.sample | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
> index a3e30bf54..5fe72cefa 100755
> --- a/t/t7519/fsmonitor-watchman
> +++ b/t/t7519/fsmonitor-watchman
> @@ -41,7 +41,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
>   $git_work_tree =~ s/[\r\n]+//g;
>   $git_work_tree =~ s,\\,/,g;
>  } else {
> - $git_work_tree = $ENV{'PWD'};
> + require Cwd;
> + $git_work_tree = Cwd::cwd();
>  }
>  
>  my $retry = 1;
> diff --git a/templates/hooks--fsmonitor-watchman.sample 
> b/templates/hooks--fsmonitor-watchman.sample
> index 9a082f278..ba6d88c5f 100755
> --- a/templates/hooks--fsmonitor-watchman.sample
> +++ b/templates/hooks--fsmonitor-watchman.sample
> @@ -40,7 +40,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
>   $git_work_tree =~ s/[\r\n]+//g;
>   $git_work_tree =~ s,\\,/,g;
>  } else {
> - $git_work_tree = $ENV{'PWD'};
> + require Cwd;
> + $git_work_tree = Cwd::cwd();
>  }
>  
>  my $retry = 1;

Re: [Query] Separate hooks for Git worktrees

[Junio C Hamano]

Stefan Beller  writes:

> We have no worktree specific config yet, though patches for
> this were floated on the mailing list.
>
> Though recent versions of git learned to conditionally include
> config files. (look for includeIf in man git-config), which I think
> could be used to set the option gerrit.createChangeId  depending
> on the worktree you are in.
>
>> Any idea how I can get around this problem without having separate
>> repositories for kernel and android ?
>
> The proposed approach above might be hacky but sounds as if
> it should work?

If you meant "conditional include" by "proposed approach above", I
do not see which part you found possibly hacky.  It is to allow
different set of configurations to apply depending on where you are
working at, which I think was invented exactly for something like
this.

It certainly is not any hackier than using the same repository to
house separately manged projects even if they may be related
projects.

Where does the aversion of "having separate repositories" primarily
come from?  Is it bad due to disk consumption?  Is it bad because
you cannot do "git diff android-branch kernel-branch"?  Something
else?

If it is purely disk consumption that is an issue, perhaps the real
solution is to make it easier to maintain separate repositories
while sharing as much disk space as possible.  GC may have to be
made a lot more robust in the presense of alternate object stores,
for example.

Re: [PATCH v1 1/4] fastindex: speed up index load through parallelization

[Junio C Hamano]

Ben Peart  writes:

> To make this work for V4 indexes, when writing the index, it periodically
> "resets" the prefix-compression by encoding the current entry as if the
> path name for the previous entry is completely different and saves the
> offset of that entry in the IEOT.  Basically, with V4 indexes, it
> generates offsets into blocks of prefix-compressed entries.

You specifically mentioned about this change in your earlier
correspondence on this series, and it reminds me of Shawn's reftable
proposal that also is heavily depended on prefix compression with
occasional reset to allow scanning from an entry in the middle.  I
find it a totally sensible design choice.

> To enable reading the IEOT extension before reading all the variable
> length cache entries and other extensions, the IEOT is written last,
> right before the trailing SHA1.

OK, we have already closed the door for further enhancements to
place something other than the index extensions after this block by
mistakenly made it the rule that the end of the series of extended
sections must coincide with the beginning of the trailing checksum,
so it does not sound all that bad to insist on this particular one
to be the last, I guess.  But see below...

> The format of that extension has the signature bytes and size at the
> beginning (like a normal extension) as well as at the end in reverse
> order to enable parsing the extension by seeking back from the end of
> the file.

I think this is a reasonable workaround to allow a single extension
that needs to be read before the main index is loaded.  

But I'd suggest taking this approach one step further.  Instead,
what if we introduce a new extension EOIE ("end of index entries")
whose sole purpose is to sit at the end of the series of extensions
and point at the beginning of the index extension section of the
file, to tell you where to seek in order to skip the main index?

That way, you can

 - seek to the end of the index file;

 - go backward skiping the trailing file checksum;

 - now you might be at the end of the EOIE extension.  seek back
   necessary number of bytes and verify EOIE header, pick up the
   recorded file offset of the beginning of the extensions section.

 - The 4-byte sequence you found may happen to match EOIE but that
   is not enough to be sure that you indeed have such an extension.
   So the following must be done carefully, allowing the possibility
   that there wasn't any EOIE extension at the end.
   Seek back to that offset, and repeat these three steps to skip
   over all extensions:

   - read the (possible) 4-byte header
   - read the (possible) extsize (validate that this is a sane value)
   - skip that many bytes

   until you come back to the location you assumed that you found
   your EOIE header, to make sure you did not get fooled by bytes
   that happened to look like one.  Some "extsize" you picked up
   during that process may lead you beyond the end of the index
   file, which would be a sure sign that what you found at the end
   of the index file was *not* the EOIE extension but a part of
   some other extension who happened to have these four bytes at the
   right place.

which would be a lot more robust way to allow any extensions to be
read before the main body of the index.

And a lot more importantly, we will leave the door open to allow
more than one index extensions that we would prefer to read before
reading the main body if we do it this way, because we can easily
skip things over without spending cycles once we have a robust way
to find where the end of the main index is.  After all, the reason
you placed IEOT at the end is not because you wanted to have it at
the very end.  You only wanted to be able to find where it is
without having to parse the variable-length records of the main
index.  And there may be other index extensions that wants to be
readable without reading the main part just like IEOT, and we do not
want to close the door for them.

Hmm?

Re: [Query] Separate hooks for Git worktrees

[Viresh Kumar]

On 09-11-17, 11:14, Stefan Beller wrote:
> The proposed approach above might be hacky but sounds as if
> it should work?

Yeah its hacky for sure, but it solved my problem. Thanks for your
help Stefan :)

-- 
viresh

Re: [RFD] Long term plan with submodule refs?

[Jacob Keller]

On Thu, Nov 9, 2017 at 12:16 PM, Stefan Beller  wrote:
> On Wed, Nov 8, 2017 at 10:54 PM, Jacob Keller  wrote:
>> On Wed, Nov 8, 2017 at 4:10 PM, Stefan Beller  wrote:
 The relationship is indeed currently useful, but if the long term plan
 is to strongly discourage detached submodule HEAD, then I would think
 that these patches are in the wrong direction. (If the long term plan is
 to end up supporting both detached and linked submodule HEAD, then these
 patches are fine, of course.) So I think that the plan referenced in
 Junio's email (that you linked above) still needs to be discussed.
>>>
>>
>>> New type of symbolic refs
>>> =
>>> A symbolic ref can currently only point at a ref or another symbolic ref.
>>> This proposal showcases different scenarios on how this could change in the
>>> future.
>>>
>>> HEAD pointing at the superprojects index
>>> 
>>> Introduce a new symbolic ref that points at the superprojects
>>> index of the gitlink. The format is
>>>
>>>   "repo:"  '\0'  '\0'
>>>
>>> Just like existing symrefs, the content of the ref will be read and 
>>> followed.
>>> On reading "repo:", the sha1 will be obtained equivalent to:
>>>
>>> git -C  ls-files -s  | awk '{ print $2}'
>>>
>>> Ref write operations driven by the submodule, affecting symrefs
>>>   e.g. git checkout  (in the submodule)
>>>
>>> In this scenario only the HEAD is optionally attached to the superproject,
>>> so we can rewrite the HEAD to be anything else, such as a branch just fine.
>>> Once the HEAD is not pointing at the superproject any more, we'll leave the
>>> submodule alone in operations driven by the superproject.
>>> To get back on the superproject branch, we’d need to invent new UX, such as
>>>git checkout --attach-superproject
>>> as that is similar to --detach
>>>
>>
>> Some of the idea trimmed for brevity, but I like this aspect the most.
>> Currently, I work on several projects which have multiple
>> repositories, which are essentially submodules.
>>
>> However, historically, we kept them separate. 99% of the time, you can
>> use all 3 projects on "master" and everything works. But if you go
>> back in time, there's no correlation to "what did the parent project
>> want this "COMMON" folder to be at?
>
> So an environment where "git submodule update --remote" is not that
> harmful, but rather brings the joy of being up to date in each project?
>
>> I started promoting using submodules for this, since it seemed quite natural.
>>
>> The core problem, is that several developers never quite understood or
>> grasped how submodules worked. There's problems like "but what if I
>> wanna work on master?" or people assume submodules need to be checked
>> out at master instead of in a detached HEAD state.
>
> So the documentation sucks?
>
> It is intentional that from the superprojects perspective the gitlink
> must be one
> exact value, and rely on the submodule to get to and keep that state.
>
> (I think we once discussed if setting the gitlink value to 00...00 or 
> otherwise
> signal that we actually want "the most recent tip of the X branch" would be
> a good idea, but I do not think it as it misses the point of versioning)
>
>> So we often get people who don't run git submodule update and thus are
>> confused about why their submodules are often out of date. (This can
>> be solved by recursive options to commands to more often recurse into
>> submodules and checkout and update them).
>>
>> We also often get people who accidentally commit the old version of
>> the repository, or commit an update to the parent project pointing the
>> submodule at some commit which isn't yet in the upstream of the common
>> repository.
>
> Would an upstream prereceive hook (maybe even builtin and accessible via
> 'receive.denyUnreachableSubmodules') help? (It would require submodules
> to be defined with relative URLs in the .gitmodules file and then the receive
> command can check for the gitlink value present in this other repository)
>
>> The proposal here seems to match the intuition about how submodules
>> should work, with the ability to "attach" or "detach" the submodule
>> when working on the submodule directly.
>
> Well I think the big picture discussion is how easy this attaching or
> detaching is. Whether only the HEAD is attached or detached, or if we
> invent a new refstore that is a complete new submodule thing, which
> cannot be detached from the superproject at all.
>
>> Ideally, I'd like for more ways to say "ignore what my submodule is
>> checked out at, since I will have something else checked out, and
>> don't intend to commit just yet."
>
> This is in the superproject, when doing a git add . ?
>

Yes.

>> Basically, a workflow where it's easier to have each submodule checked
>> out at master, and we can still keep track of historical relationship
>> of what 

Re: [PATCH] notes: add `rm` and `delete` commands

[Junio C Hamano]

Eric Sunshine  writes:

> or against the change:
>
> - synonym bloat; balloons documentation
> - steals command verbs from potential future features
> - ...

I tend to agree with these two (and if I were to replace the "..."
with something concrete, this change is not desirable because it
will force us to add all these three when we introduce "remove"
elsewhere in the system).

Having said that, this remids me of an age-old topic we discussed in
a distant past but stalled due to lack of strong drive, which is:

Some git subcommands take command verb (e.g. "git notes 'remove'")
while others take command mode flags (e.g. "git branch --delete"),
and the users need to remember them, which is not ideal.

I think the consensus back then, if we were to aim for consistency
by uniformly using only one of the two, is to use the command mode
flags, simply because existing commands that have the primary mode
and take an optional command mode flag [*1*] cannot be migrated to
the command verb UI safely and sanely.

And then, if we are not careful when we standardize all subcommands
to take command mode flags, we might end up with a situation where
some subcommand say "--remove" while other say "--delete", and some
users will wish to rectify that situation by adding an alias defined
for these flags---I view this patch as a part of that possible
future topic ;-).


[Footnote]

*1* For example, "git branch" by default is in the branch creation
mode, but it can be told to work in its branch deletion mode
with "--delete".

If we were to standardize on command verbs, is "git branch
delete" a deprecated relic from the old world that wants to
create a branch whose name is 'delete', or is it done by a
script in the new world that collected names of branches to
delete and asked "git branch delete $to_delete" to delete all of
them, where the $to_delete variable happened to end up empty?

With command mode flags, we do not have to worry about such an
ambiguity during and after transition.

Re: [PATCH 6/7] builtin/describe.c: describe a blob

[Junio C Hamano]

"Philip Oakley"  writes:

> From: "Stefan Beller" 
>> Rereading this discussion, there is currently no urgent thing to address?
>
> True.
>
>> Then the state as announced by the last cooking email, to just cook
>> it, seems
>> about right and we'll wait for further feedback.

A shiny new toy that is not a fix for a grave bug is rarely urgent,
so with that criterion, we'd end up with hundreds of topics not in
'next' but in 'pu' waiting for the original contributor to get out
of his or her procrastination, which certainly is not what I want to
see, as I'd have to throw them into the Stalled bin and then
eventually discard them, while having to worry about possible
mismerges with remaining good topics caused by these topics
appearing and disappearing from 'pu'.

I'd rather see any topic that consumed reviewers' time to be
polished enough to get into 'next' while we all recall the issues
raised during previous reviews.  I consider the process to further
incrementally polish it after that happens a true "cooking".

For this topic, aside from "known issues" that we decided to punt
for now, my impression was that the code is in good enough shape,
and we need a bit of documentation polishes before I can mark it
as "Will merge to 'next'".

> Possibly only checking the documenation aspects, so folks don't fall
> into the same trap as me.. ;-)

Yup, so let's resolve that documentation thing while we remember
that the topic has that issue, and what part of the documentation
we find needs improvement.

I am not sure what "trap: you fell into, though.  Are you saying
that giving

git describe [...] 
git describe [...] 

in the synopsis is not helpful, because the user may not know what
kind of object s/he has, and cannot decide from which set of options
to pick?  Then an alternative would be to list

git describe [...] 

in the synopsis, say upfront that most options are applicable only
when describing a commit-ish, and when describing a blob, we do
quite different thing and a separate set of options apply, perhaps?

[PATCH 2/4] builtin/blame: dim uninteresting metadata

[Stefan Beller]

When using git-blame lots of lines contain redundant information, for
example in hunks that consist of multiple lines, the metadata (commit
name, author, date) are repeated. A reader may not be interested in those,
so offer an option to color the information that is repeated from the
previous line differently.

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt |  5 
 builtin/blame.c  | 68 ++--
 color.h  |  1 +
 t/t8012-blame-colors.sh  | 18 +
 4 files changed, 78 insertions(+), 14 deletions(-)
 create mode 100755 t/t8012-blame-colors.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f0d62753d..85dfdc6a9b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1189,6 +1189,11 @@ color.status.::
status short-format), or
`unmerged` (files which have unmerged changes).
 
+color.blame.repeatedMeta::
+   Use the customized color for the part of git-blame output that
+   is repeated meta information per line (such as commit id,
+   author name, date and timezone). Defaults to dark gray.
+
 color.ui::
This variable determines the default value for variables such
as `color.diff` and `color.grep` that control the use of color
diff --git a/builtin/blame.c b/builtin/blame.c
index 67adaef4d8..d15e901357 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -7,6 +7,7 @@
 
 #include "cache.h"
 #include "config.h"
+#include "color.h"
 #include "builtin.h"
 #include "commit.h"
 #include "diff.h"
@@ -46,6 +47,7 @@ static int xdl_opts;
 static int abbrev = -1;
 static int no_whole_file_rename;
 static int show_progress;
+static char *repeated_meta_color;
 
 static struct date_mode blame_date_mode = { DATE_ISO8601 };
 static size_t blame_date_width;
@@ -316,10 +318,11 @@ static const char *format_time(timestamp_t time, const 
char *tz_str,
 #define OUTPUT_PORCELAIN   010
 #define OUTPUT_SHOW_NAME   020
 #define OUTPUT_SHOW_NUMBER 040
-#define OUTPUT_SHOW_SCORE  0100
-#define OUTPUT_NO_AUTHOR   0200
+#define OUTPUT_SHOW_SCORE  0100
+#define OUTPUT_NO_AUTHOR   0200
 #define OUTPUT_SHOW_EMAIL  0400
-#define OUTPUT_LINE_PORCELAIN 01000
+#define OUTPUT_LINE_PORCELAIN  01000
+#define OUTPUT_COLOR_LINE  02000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -367,6 +370,28 @@ static void emit_porcelain(struct blame_scoreboard *sb, 
struct blame_entry *ent,
putchar('\n');
 }
 
+static inline void colors_unset(const char **use_color, const char 
**reset_color)
+{
+   *use_color = "";
+   *reset_color = "";
+}
+
+static inline void colors_set(const char **use_color, const char **reset_color)
+{
+   *use_color = repeated_meta_color;
+   *reset_color = GIT_COLOR_RESET;
+}
+
+static void setup_line_color(int opt, int cnt,
+const char **use_color,
+const char **reset_color)
+{
+   colors_unset(use_color, reset_color);
+
+   if ((opt & OUTPUT_COLOR_LINE) && cnt > 0)
+   colors_set(use_color, reset_color);
+}
+
 static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, 
int opt)
 {
int cnt;
@@ -383,6 +408,7 @@ static void emit_other(struct blame_scoreboard *sb, struct 
blame_entry *ent, int
for (cnt = 0; cnt < ent->num_lines; cnt++) {
char ch;
int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : 
abbrev;
+   const char *col, *rcol;
 
if (suspect->commit->object.flags & UNINTERESTING) {
if (blank_boundary)
@@ -393,7 +419,8 @@ static void emit_other(struct blame_scoreboard *sb, struct 
blame_entry *ent, int
}
}
 
-   printf("%.*s", length, hex);
+   setup_line_color(opt, cnt, , );
+   printf("%s%.*s%s", col, length, hex, rcol);
if (opt & OUTPUT_ANNOTATE_COMPAT) {
const char *name;
if (opt & OUTPUT_SHOW_EMAIL)
@@ -406,16 +433,17 @@ static void emit_other(struct blame_scoreboard *sb, 
struct blame_entry *ent, int
   ent->lno + 1 + cnt);
} else {
if (opt & OUTPUT_SHOW_SCORE)
-   printf(" %*d %02d",
+   printf(" %s%*d %02d%s", col,
   max_score_digits, ent->score,
-  ent->suspect->refcnt);
+  ent->suspect->refcnt, rcol);
if (opt & OUTPUT_SHOW_NAME)
-   printf(" %-*.*s", longest_file, longest_file,
-  suspect->path);
+   printf(" %s%-*.*s%s", col, longest_file,
+  

[PATCH 3/4] builtin/blame: add option to color metadata fields separately

[Stefan Beller]

Unlike the previous commit, this dims colors for each
metadata field individually.

Signed-off-by: Stefan Beller 
---
 builtin/blame.c | 82 +++--
 t/t8012-blame-colors.sh | 38 +++
 2 files changed, 111 insertions(+), 9 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index d15e901357..9d460597a2 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -323,6 +323,7 @@ static const char *format_time(timestamp_t time, const char 
*tz_str,
 #define OUTPUT_SHOW_EMAIL  0400
 #define OUTPUT_LINE_PORCELAIN  01000
 #define OUTPUT_COLOR_LINE  02000
+#define OUTPUT_COLOR_FIELDS04000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -370,6 +371,33 @@ static void emit_porcelain(struct blame_scoreboard *sb, 
struct blame_entry *ent,
putchar('\n');
 }
 
+static int had_same_field_previously(int opt, int field,
+ struct blame_entry *ent,
+ struct blame_entry *prev)
+{
+   struct commit_info ci, prev_ci;
+
+   switch (field) {
+   case OUTPUT_SHOW_SCORE:
+   return ent->score == prev->score;
+   case OUTPUT_SHOW_NAME:
+   return prev->suspect &&
+   !strcmp(ent->suspect->path, prev->suspect->path);
+   case OUTPUT_SHOW_NUMBER:
+   return ent->s_lno == prev->s_lno + prev->num_lines - 1;
+
+   case OUTPUT_NO_AUTHOR:
+   get_commit_info(ent->suspect->commit, , 1);
+   get_commit_info(prev->suspect->commit, _ci, 1);
+   return ((opt & OUTPUT_SHOW_EMAIL) &&
+   !strcmp(ci.author_mail.buf, prev_ci.author_mail.buf)) ||
+   !strcmp(ci.author.buf, prev_ci.author.buf);
+   default:
+   BUG("unknown field");
+   }
+   return 0;
+}
+
 static inline void colors_unset(const char **use_color, const char 
**reset_color)
 {
*use_color = "";
@@ -382,17 +410,36 @@ static inline void colors_set(const char **use_color, 
const char **reset_color)
*reset_color = GIT_COLOR_RESET;
 }
 
+static void setup_field_color(int opt, int cnt, int field,
+ struct blame_entry *ent,
+ struct blame_entry *prev,
+ const char **use_color,
+ const char **reset_color)
+{
+   if (!(opt & OUTPUT_COLOR_FIELDS))
+   return;
+
+   if ((cnt > 0 ||
+(prev && had_same_field_previously(opt, field, ent, prev
+   colors_set(use_color, reset_color);
+   else
+   colors_unset(use_color, reset_color);
+}
+
 static void setup_line_color(int opt, int cnt,
 const char **use_color,
 const char **reset_color)
 {
colors_unset(use_color, reset_color);
 
-   if ((opt & OUTPUT_COLOR_LINE) && cnt > 0)
+   if ((opt & (OUTPUT_COLOR_LINE | OUTPUT_COLOR_FIELDS)) && cnt > 0)
colors_set(use_color, reset_color);
 }
 
-static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, 
int opt)
+static void emit_other(struct blame_scoreboard *sb,
+  struct blame_entry *ent,
+  struct blame_entry *prev,
+  int opt)
 {
int cnt;
const char *cp;
@@ -432,18 +479,27 @@ static void emit_other(struct blame_scoreboard *sb, 
struct blame_entry *ent, int
   show_raw_time),
   ent->lno + 1 + cnt);
} else {
-   if (opt & OUTPUT_SHOW_SCORE)
+   if (opt & OUTPUT_SHOW_SCORE) {
+   setup_field_color(opt, cnt, OUTPUT_SHOW_SCORE,
+ ent, prev, , );
printf(" %s%*d %02d%s", col,
   max_score_digits, ent->score,
   ent->suspect->refcnt, rcol);
-   if (opt & OUTPUT_SHOW_NAME)
+   }
+   if (opt & OUTPUT_SHOW_NAME) {
+   setup_field_color(opt, cnt, OUTPUT_SHOW_NAME,
+  ent, prev, , );
printf(" %s%-*.*s%s", col, longest_file,
  longest_file,
  suspect->path,
  rcol);
-   if (opt & OUTPUT_SHOW_NUMBER)
+   }
+   if (opt & OUTPUT_SHOW_NUMBER) {
+   setup_field_color(opt, cnt, OUTPUT_SHOW_NUMBER,
+ ent, prev, , );
 

[PATCH 4/4] builtin/blame: highlight recently changed lines

[Stefan Beller]

Choose a different color for dates and imitate a 'temperature cool down'
for the dates.

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt | 20 +
 builtin/blame.c  | 74 +++-
 2 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 85dfdc6a9b..7d2efc5ad6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1194,6 +1194,26 @@ color.blame.repeatedMeta::
is repeated meta information per line (such as commit id,
author name, date and timezone). Defaults to dark gray.
 
+color.blame.highlightRecent::
+   This can be used to color the author and date of a blame line.
+   This overrides `color.blame.repeatedMeta` setting, which colors
+   repetitions.
++
+   This setting should be set to a comma-separated list of
+   color and date settings, starting and ending with a color, the dates
+   should be set from oldest to newest.
+   The metadata will be colored given the colors if the the line was
+   introduced before the given timestamp, overwriting older timestamped
+   colors.
++
+   Instead of an absolute timestamp relative timestamps work as
+   well, e.g. 2.weeks.ago is valid to address anything older than 2 weeks.
++
+   It defaults to "blue,12 month ago,white,1 month ago,red",
+   which colors everything older than one year blue, recent changes between
+   one month and one year old are kept white, and lines introduced within
+   the last month are colored red.
+
 color.ui::
This variable determines the default value for variables such
as `color.diff` and `color.grep` that control the use of color
diff --git a/builtin/blame.c b/builtin/blame.c
index 9d460597a2..e157ee3864 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -24,6 +24,7 @@
 #include "dir.h"
 #include "progress.h"
 #include "blame.h"
+#include "string-list.h"
 
 static char blame_usage[] = N_("git blame [] [] [] 
[--] ");
 
@@ -324,6 +325,7 @@ static const char *format_time(timestamp_t time, const char 
*tz_str,
 #define OUTPUT_LINE_PORCELAIN  01000
 #define OUTPUT_COLOR_LINE  02000
 #define OUTPUT_COLOR_FIELDS04000
+#define OUTPUT_HEATED_LINES01
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -436,6 +438,61 @@ static void setup_line_color(int opt, int cnt,
colors_set(use_color, reset_color);
 }
 
+static struct color_field {
+   timestamp_t hop;
+   char col[COLOR_MAXLEN];
+} *colorfield;
+static int colorfield_nr, colorfield_alloc;
+
+static void parse_color_fields(const char *s)
+{
+   struct string_list l = STRING_LIST_INIT_DUP;
+   struct string_list_item *item;
+   enum { EXPECT_DATE, EXPECT_COLOR } next = EXPECT_COLOR;
+
+   /* Ideally this would be stripped and split at the same time? */
+   string_list_split(, s, ',', -1);
+   ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
+
+   for_each_string_list_item(item, ) {
+   switch (next) {
+   case EXPECT_DATE:
+   colorfield[colorfield_nr].hop = 
approxidate(item->string);
+   next = EXPECT_COLOR;
+   colorfield_nr++;
+   ALLOC_GROW(colorfield, colorfield_nr + 1, 
colorfield_alloc);
+   break;
+   case EXPECT_COLOR:
+   if (color_parse(item->string, 
colorfield[colorfield_nr].col))
+   die(_("expecting a color: %s"), item->string);
+   next = EXPECT_DATE;
+   break;
+   }
+   }
+
+   if (next == EXPECT_COLOR)
+   die (_("must end with a color"));
+
+   colorfield[colorfield_nr].hop = TIME_MAX;
+}
+
+static void setup_default_colorfield(void)
+{
+   parse_color_fields("blue,12 month ago,white,1 month ago,red");
+}
+
+static void determine_line_heat(struct blame_entry *ent, const char 
**dest_color)
+{
+   int i = 0;
+   struct commit_info ci;
+   get_commit_info(ent->suspect->commit, , 1);
+
+   while (i < colorfield_nr && ci.author_time > colorfield[i].hop)
+   i++;
+
+   *dest_color = colorfield[i].col;
+}
+
 static void emit_other(struct blame_scoreboard *sb,
   struct blame_entry *ent,
   struct blame_entry *prev,
@@ -443,6 +500,7 @@ static void emit_other(struct blame_scoreboard *sb,
 {
int cnt;
const char *cp;
+   const char *heatcolor = NULL;
struct blame_origin *suspect = ent->suspect;
struct commit_info ci;
char hex[GIT_MAX_HEXSZ + 1];
@@ -452,6 +510,10 @@ static void emit_other(struct blame_scoreboard *sb,
oid_to_hex_r(hex, >commit->object.oid);
 
cp = blame_nth_line(sb, ent->lno);
+
+   if (opt & 

[RFC PATCH 0/4] blame: (dim rep. metadata lines or fields, decay date coloring)

[Stefan Beller]

This is picking up [1], but presenting it in another approach,
as I realized these are orthogonal features:
* dimming repeated lines/fields of information
* giving a quick visual information how old (as a proxy for 'well tested')
  a line of code is.
  
Both features are configurable.

Any feedback welcome.

Thanks,
Stefan

[1] https://public-inbox.org/git/20170613023151.9688-1-sbel...@google.com/

Stefan Beller (4):
  color.h: modernize header
  builtin/blame: dim uninteresting metadata
  builtin/blame: add option to color metadata fields separately
  builtin/blame: highlight recently changed lines

 Documentation/config.txt |  25 ++
 builtin/blame.c  | 216 ++-
 color.c  |   2 -
 color.h  |  49 ---
 t/t8012-blame-colors.sh  |  56 
 5 files changed, 313 insertions(+), 35 deletions(-)
 create mode 100755 t/t8012-blame-colors.sh

-- 
2.15.0.128.gcadd42da22

[PATCH 1/4] color.h: modernize header

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 color.c |  2 --
 color.h | 48 +++-
 2 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/color.c b/color.c
index 9a9261ac16..e43da3ce53 100644
--- a/color.c
+++ b/color.c
@@ -400,8 +400,6 @@ static int color_vfprintf(FILE *fp, const char *color, 
const char *fmt,
return r;
 }
 
-
-
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
 {
va_list args;
diff --git a/color.h b/color.h
index fd2b688dfb..6cd632c0d8 100644
--- a/color.h
+++ b/color.h
@@ -72,26 +72,48 @@ extern int color_stdout_is_tty;
  * Use the first one if you need only color config; the second is a convenience
  * if you are just going to change to git_default_config, too.
  */
-int git_color_config(const char *var, const char *value, void *cb);
-int git_color_default_config(const char *var, const char *value, void *cb);
+extern int git_color_config(const char *var, const char *value, void *cb);
+extern int git_color_default_config(const char *var, const char *value, void 
*cb);
 
 /*
- * Set the color buffer (which must be COLOR_MAXLEN bytes)
- * to the raw color bytes; this is useful for initializing
+ * Set the color buffer `dst` (which must be COLOR_MAXLEN bytes)
+ * to the raw color bytes `color_bytes`; this is useful for initializing
  * default color variables.
  */
-void color_set(char *dst, const char *color_bytes);
+extern void color_set(char *dst, const char *color_bytes);
 
-int git_config_colorbool(const char *var, const char *value);
-int want_color(int var);
-int color_parse(const char *value, char *dst);
-int color_parse_mem(const char *value, int len, char *dst);
+/*
+ * Parses a config option, which can be a boolean or one of
+ * "never", "auto", "always". Returns the constant for the given setting.
+ */
+extern int git_config_colorbool(const char *var, const char *value);
+
+/* Is the output supposed to be colored? Resolve and cache the 'auto' setting 
*/
+extern int want_color(int var);
+
+/*
+ * Translate the human readable color string from `value` and into
+ * terminal color codes and store them in `dst`
+ */
+extern int color_parse(const char *value, char *dst);
+extern int color_parse_mem(const char *value, int len, char *dst);
+
+/*
+ * Print the format string `fmt`, encapsulated by setting and resetting the
+ * color. Omits the color encapsulation if `color` is NULL.
+ * The `color_fprintf_ln` prints a new line after resetting the color.
+ * The `color_print_strbuf` prints the given pre-formatted strbuf instead.
+ */
 __attribute__((format (printf, 3, 4)))
-int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
+extern int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
-int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
-void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
+extern int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
+extern void color_print_strbuf(FILE *fp, const char *color, const struct 
strbuf *sb);
 
-int color_is_nil(const char *color);
+/*
+ * Check if the given color is GIT_COLOR_NIL that means "no color selected".
+ * The application needs to replace the color with the actual desired color.
+ */
+extern int color_is_nil(const char *color);
 
 #endif /* COLOR_H */
-- 
2.15.0.128.gcadd42da22

Re: Bug - Status - Space in Filename

[Junio C Hamano]

Jeff King  writes:

> Yeah, I think the original sin is using " -> " in the --porcelain output
> (which just got it from --short). That should have been a HT all along
> to match the rest of the diff code. The --porcelain=v2 format gets this
> right.

So we at lesat did something right, which is a consolation.

>> Perhaps a better approach is to refactor the extra quoting I moved
>> to this emit_with_optional_dq() helper into quote_path() which
>> currently is just aliased to quote_path_relative().  It also is
>> possible that we may want to extend the "no_dq" parameter that is
>> given to quote_c_style() helper from a boolean to a set of flag
>> bits, and allow callers to request "I want SP added to the set of
>> bytes that triggers the quoting".
>
> Yeah, I had the same thought upon digging into the code.
>
> That said, if this is the only place that has this funny quoting, it may
> not be worth polluting the rest of the code with the idea that quoting
> spaces is a good thing to do.

Sounds sane.  We can probably use a helper like this:

static char *quote_path_with_sp(const char *in, const char *prefix, struct 
strbuf *out)
{
const char dq = '"';

quote_path(in, prefix, out);
if (out->buf[0] != dq && strchr(out->buf, ' ') != NULL) {
strbuf_insert(out, 0, , 1);
strbuf_addch(out, dq);
}
return out->buf;
}

which allows the current users like shortstatus_status() to become a
lot shorter.

Re: [PATCH 6/7] builtin/describe.c: describe a blob

[Philip Oakley]

From: "Stefan Beller" 

Rereading this discussion, there is currently no urgent thing to address?


True.

Then the state as announced by the last cooking email, to just cook it, 
seems

about right and we'll wait for further feedback.


Possibly only checking the documenation aspects, so folks don't fall into 
the same trap as me.. ;-)

--
Philip 

Re: [PATCH v2 1/1] Introduce git add --renormalize .

[Junio C Hamano]

Torsten Bögershausen  writes:

> Here is a somwhat shorter description:
>
> Apply the "clean" process freshly to all tracked files.
> This is useful after changing `core.autocrlf` or the `text`
> attributes in the `.gitattributes` file because
> Git may not consider these files as changed.

I think it is OK to omit .git/config for brevity (I am assuming that
your justification is because you thought it was obvious it is a
configuration variable); but then it is equally obvious (if not
more) that `text` attribute comes from .gitattributes (notice we do
not mention core.autocrlf is a configuration variable in the above,
but we do say `text` is an attribute) so it can also be omitted for
brevity.

> Correct the files that had been commited with CRLF,
> they will from now on have LF instead.

Reading this as a single sentence immediately after the above
paragraph leaves me feel confused.  First of all, this would not
happen unless the user corrects core.autocrlf/text like described
above.  In fact, updating these settings is done as in order to do
that correction.  So I'd say it should not be split.

> Re-run what the `clean` filter does.

This again looks out of place just like the previous sentence.  In
fact, provided if "the clean process" is understood by the end user,
this is redundant.

> This option implies `-u`.

Taking these altogether, perhaps

Apply the "clean" process freshly to all tracked files to
forcibly add them again to the index.  This is useful after
changing `core.autocrlf` configuration or the `text` attribute
in order to correct files added with wrong CRLF/LF line endings.
This option implies `-u`.

Thanks.

Re: [PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish

[Philip Oakley]

From: "Junio C Hamano" 
Sent: Wednesday, November 08, 2017 1:59 AM



"Philip Oakley"  writes:


But...

...
This change causes quite a few tests to fall over; however, they
all have truncated-something-longer-ellipses in their
raw-diff-output expected sections, and removing the ellipses
from there makes the tests pass again, :-)


The number of failures you report in the test suit suggests that
someone somewhere will be expecting that notation, and that we may
need a deprecation period, perhaps with an 'ellipsis' config variable
whose default value can later be flipped, though that leaves a config
value needing support forever!


Hmmm, never thought about that.

I have been assuming that tools reading "--raw" output that is
abbreviated would be crazy, because they have to strip the dots and
the number of dots may not always be three [*1*].

But you are right.  It would be very unlikely that there is no such
crazy tools, so it deserves consideration if we would be breaking
such tools.

On the other hand, if such a crazy tool was still written correctly
(it is debatable what the definition of "correct" is, though), it
would be stripping any number dots at the end, not just insisting on
seeing exactly three dots, and splitting these fields at SP.
Otherwise they would already be broken as they cannot handle
occasional object names that have less than three dots because they
happen to be longer than the more common abbreviation length used by
other objects.  So in practice it might not be _too_ bad.

Thinking on this, I'd suggest that the patch series does remove the ellipsis 
dots immediately, but retains a config option that can be set to get back 
the old 'dots' display for those who have badly written scripts that maybe 
haven't failed yet. i.e. no deprecation period, just a fall back option; and 
if nobody shouts then remove the config option after a respectable period.


It would also mean the existing tests can be re-used...



[Footnote]

*1* When we ask for --abbrev=7, we allocate 10 places and fill the
rest with necessary number of dots after the result of
find_unique_abbrev(), so if an object name turns out to require 8
hexdigits to make it unique, we'll append only two dots to it to
make it 10 so that it aligns nicely with others) and they would
always be reading the full, non abbreviated output.  The story does
not change that much when we do not explicitly ask for a specific
abbreviation length in that we add variable number of dots for
aligning in that case, too.


The --abbrev=7 does cater for many smaller repo's, so there is a possiblity 
that the bad script issue hasn't been hit yet by those repos.

--

Philip 

[PATCH] contrib/git-jump: allow to configure the grep command

[Beat Bolli]

Add the configuration option "jump.grepCmd" that allows to configure the
command that is used to search in grep mode. This allows the users of
git-jump to use ag(1) or ack(1) as search engines.

Signed-off-by: Beat Bolli 
---
 contrib/git-jump/README   | 3 +++
 contrib/git-jump/git-jump | 7 +--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 225e3f095..9f58d5db8 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -63,6 +63,9 @@ git jump grep foo_bar
 # same as above, but case-insensitive; you can give
 # arbitrary grep options
 git jump grep -i foo_bar
+
+# use the silver searcher for git jump grep
+git config jump.grepCmd "ag --column"
 --
 
 
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 427f206a4..80ab0590b 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -11,7 +11,8 @@ diff: elements are diff hunks. Arguments are given to diff.
 
 merge: elements are merge conflicts. Arguments are ignored.
 
-grep: elements are grep hits. Arguments are given to grep.
+grep: elements are grep hits. Arguments are given to git grep or, if
+  configured, to the command in `jump.grepCmd`.
 
 ws: elements are whitespace errors. Arguments are given to diff --check.
 EOF
@@ -50,7 +51,9 @@ mode_merge() {
 # but let's clean up extra whitespace, so they look better if the
 # editor shows them to us in the status bar.
 mode_grep() {
-   git grep -n "$@" |
+   cmd=$(git config jump.grepCmd)
+   test -n "$cmd" || cmd="git grep -n"
+   $cmd "$@" |
perl -pe '
s/[ \t]+/ /g;
s/^ *//;
-- 
2.15.0.rc1.299.gda03b47c3

Re: [PATCH 6/7] builtin/describe.c: describe a blob

[Stefan Beller]

Rereading this discussion, there is currently no urgent thing to address?
Then the state as announced by the last cooking email, to just cook it, seems
about right and we'll wait for further feedback.

Thanks,
Stefan

Re: [RFD] Long term plan with submodule refs?

[Stefan Beller]

On Wed, Nov 8, 2017 at 10:54 PM, Jacob Keller  wrote:
> On Wed, Nov 8, 2017 at 4:10 PM, Stefan Beller  wrote:
>>> The relationship is indeed currently useful, but if the long term plan
>>> is to strongly discourage detached submodule HEAD, then I would think
>>> that these patches are in the wrong direction. (If the long term plan is
>>> to end up supporting both detached and linked submodule HEAD, then these
>>> patches are fine, of course.) So I think that the plan referenced in
>>> Junio's email (that you linked above) still needs to be discussed.
>>
>
>> New type of symbolic refs
>> =
>> A symbolic ref can currently only point at a ref or another symbolic ref.
>> This proposal showcases different scenarios on how this could change in the
>> future.
>>
>> HEAD pointing at the superprojects index
>> 
>> Introduce a new symbolic ref that points at the superprojects
>> index of the gitlink. The format is
>>
>>   "repo:"  '\0'  '\0'
>>
>> Just like existing symrefs, the content of the ref will be read and followed.
>> On reading "repo:", the sha1 will be obtained equivalent to:
>>
>> git -C  ls-files -s  | awk '{ print $2}'
>>
>> Ref write operations driven by the submodule, affecting symrefs
>>   e.g. git checkout  (in the submodule)
>>
>> In this scenario only the HEAD is optionally attached to the superproject,
>> so we can rewrite the HEAD to be anything else, such as a branch just fine.
>> Once the HEAD is not pointing at the superproject any more, we'll leave the
>> submodule alone in operations driven by the superproject.
>> To get back on the superproject branch, we’d need to invent new UX, such as
>>git checkout --attach-superproject
>> as that is similar to --detach
>>
>
> Some of the idea trimmed for brevity, but I like this aspect the most.
> Currently, I work on several projects which have multiple
> repositories, which are essentially submodules.
>
> However, historically, we kept them separate. 99% of the time, you can
> use all 3 projects on "master" and everything works. But if you go
> back in time, there's no correlation to "what did the parent project
> want this "COMMON" folder to be at?

So an environment where "git submodule update --remote" is not that
harmful, but rather brings the joy of being up to date in each project?

> I started promoting using submodules for this, since it seemed quite natural.
>
> The core problem, is that several developers never quite understood or
> grasped how submodules worked. There's problems like "but what if I
> wanna work on master?" or people assume submodules need to be checked
> out at master instead of in a detached HEAD state.

So the documentation sucks?

It is intentional that from the superprojects perspective the gitlink
must be one
exact value, and rely on the submodule to get to and keep that state.

(I think we once discussed if setting the gitlink value to 00...00 or otherwise
signal that we actually want "the most recent tip of the X branch" would be
a good idea, but I do not think it as it misses the point of versioning)

> So we often get people who don't run git submodule update and thus are
> confused about why their submodules are often out of date. (This can
> be solved by recursive options to commands to more often recurse into
> submodules and checkout and update them).
>
> We also often get people who accidentally commit the old version of
> the repository, or commit an update to the parent project pointing the
> submodule at some commit which isn't yet in the upstream of the common
> repository.

Would an upstream prereceive hook (maybe even builtin and accessible via
'receive.denyUnreachableSubmodules') help? (It would require submodules
to be defined with relative URLs in the .gitmodules file and then the receive
command can check for the gitlink value present in this other repository)

> The proposal here seems to match the intuition about how submodules
> should work, with the ability to "attach" or "detach" the submodule
> when working on the submodule directly.

Well I think the big picture discussion is how easy this attaching or
detaching is. Whether only the HEAD is attached or detached, or if we
invent a new refstore that is a complete new submodule thing, which
cannot be detached from the superproject at all.

> Ideally, I'd like for more ways to say "ignore what my submodule is
> checked out at, since I will have something else checked out, and
> don't intend to commit just yet."

This is in the superproject, when doing a git add . ?

> Basically, a workflow where it's easier to have each submodule checked
> out at master, and we can still keep track of historical relationship
> of what commit was the submodule at some time ago, but without causing
> some of these headaches.

So essentially a repo or otherwise parallel workflow just with the versioning
happening magically behind your back?

> I've 

[PATCH 0/2] fsmonitor: Stop reading from PWD, write fsmonitor+split index right

[Alex Vandiver]

A couple further patches for the fsmonitor branch, which ideally I'd
have noticed before my previous series landed.

In the first patch I believe I've found the underlying reason for the
PWD confusion in the previous go-around -- but I'm not sure I'm wholly
convinced about my solution to it.  Specifically, it seems like this
problem is likely more widespread than this one place, so adjusting it
in the example hook may just be leaving the same dangerous edge for
others to stumble across later.

 - Alex

[PATCH 2/2] fsmonitor: Store fsmonitor bitmap before splitting index

[Alex Vandiver]

ba1b9caca6 resolved the problem of the fsmonitor data being applied to
the non-base index when reading; however, a similar problem exists
when writing the index.  Specifically, writing of the fsmonitor
extension happens only after the work to split the index has been
applied -- as such, the information in the index is only for the
non-"base" index, and thus the extension information contains only
partial data.

When saving, compute the ewah bitmap before the index is split, and
store it in the fsmonitor_dirty field, mirroring the behavior that
occurred during reading.  fsmonitor_dirty is kept from being leaked by
being freed when the extension data is written -- which always happens
precisely once, no matter the split index configuration.

Signed-off-by: Alex Vandiver 
---
 fsmonitor.c | 20 
 fsmonitor.h |  9 -
 read-cache.c|  3 +++
 t/t7519-status-fsmonitor.sh | 13 +
 4 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/fsmonitor.c b/fsmonitor.c
index f494a866d..0af7c4edb 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -54,12 +54,19 @@ int read_fsmonitor_extension(struct index_state *istate, 
const void *data,
return 0;
 }
 
+void fill_fsmonitor_bitmap(struct index_state *istate)
+{
+   int i;
+   istate->fsmonitor_dirty = ewah_new();
+   for (i = 0; i < istate->cache_nr; i++)
+   if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
+   ewah_set(istate->fsmonitor_dirty, i);
+}
+
 void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 {
uint32_t hdr_version;
uint64_t tm;
-   struct ewah_bitmap *bitmap;
-   int i;
uint32_t ewah_start;
uint32_t ewah_size = 0;
int fixup = 0;
@@ -73,12 +80,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct 
index_state *istate)
strbuf_add(sb, _size, sizeof(uint32_t)); /* we'll fix this up 
later */
 
ewah_start = sb->len;
-   bitmap = ewah_new();
-   for (i = 0; i < istate->cache_nr; i++)
-   if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
-   ewah_set(bitmap, i);
-   ewah_serialize_strbuf(bitmap, sb);
-   ewah_free(bitmap);
+   ewah_serialize_strbuf(istate->fsmonitor_dirty, sb);
+   ewah_free(istate->fsmonitor_dirty);
+   istate->fsmonitor_dirty = NULL;
 
/* fix up size field */
put_be32(_size, sb->len - ewah_start);
diff --git a/fsmonitor.h b/fsmonitor.h
index 0de644e01..cd3cc0ccf 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -10,7 +10,14 @@ extern struct trace_key trace_fsmonitor;
 extern int read_fsmonitor_extension(struct index_state *istate, const void 
*data, unsigned long sz);
 
 /*
- * Write the CE_FSMONITOR_VALID state into the fsmonitor index extension.
+ * Fill the fsmonitor_dirty ewah bits with their state from the index,
+ * before it is split during writing.
+ */
+extern void fill_fsmonitor_bitmap(struct index_state *istate);
+
+/*
+ * Write the CE_FSMONITOR_VALID state into the fsmonitor index
+ * extension.  Reads from the fsmonitor_dirty ewah in the index.
  */
 extern void write_fsmonitor_extension(struct strbuf *sb, struct index_state 
*istate);
 
diff --git a/read-cache.c b/read-cache.c
index c18e5e623..7976d39d6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2529,6 +2529,9 @@ int write_locked_index(struct index_state *istate, struct 
lock_file *lock,
int new_shared_index, ret;
struct split_index *si = istate->split_index;
 
+   if (istate->fsmonitor_last_update)
+   fill_fsmonitor_bitmap(istate);
+
if (!si || alternate_index_output ||
(istate->cache_changed & ~EXTMASK)) {
if (si)
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index c6df85af5..eb2d13bbc 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -301,4 +301,17 @@ do
done
 done
 
+# test that splitting the index dosn't interfere
+test_expect_success 'splitting the index results in the same state' '
+   write_integration_script &&
+   dirty_repo &&
+   git update-index --fsmonitor  &&
+   git ls-files -f >expect &&
+   test-dump-fsmonitor >&2 && echo &&
+   git update-index --fsmonitor --split-index &&
+   test-dump-fsmonitor >&2 && echo &&
+   git ls-files -f >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.15.0.rc1.413.g76aedb451

[PATCH 1/2] fsmonitor: Read from getcwd(), not the PWD environment variable

[Alex Vandiver]

Though the process has chdir'd to the root of the working tree, the
PWD environment variable is only guaranteed to be updated accordingly
if a shell is involved -- which is not guaranteed to be the case.
That is, if `/usr/bin/perl` is a binary, $ENV{PWD} is unchanged from
whatever spawned `git` -- if `/usr/bin/perl` is a trivial shell
wrapper to the real `perl`, `$ENV{PWD}` will have been updated to the
root of the working copy.

Update to read from the Cwd module using the `getcwd` syscall, not the
PWD environment variable.  The Cygwin case is left unchanged, as it
necessarily _does_ go through a shell.

Signed-off-by: Alex Vandiver 
---
 t/t7519/fsmonitor-watchman | 3 ++-
 templates/hooks--fsmonitor-watchman.sample | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index a3e30bf54..5fe72cefa 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -41,7 +41,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
$git_work_tree =~ s/[\r\n]+//g;
$git_work_tree =~ s,\\,/,g;
 } else {
-   $git_work_tree = $ENV{'PWD'};
+   require Cwd;
+   $git_work_tree = Cwd::cwd();
 }
 
 my $retry = 1;
diff --git a/templates/hooks--fsmonitor-watchman.sample 
b/templates/hooks--fsmonitor-watchman.sample
index 9a082f278..ba6d88c5f 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -40,7 +40,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
$git_work_tree =~ s/[\r\n]+//g;
$git_work_tree =~ s,\\,/,g;
 } else {
-   $git_work_tree = $ENV{'PWD'};
+   require Cwd;
+   $git_work_tree = Cwd::cwd();
 }
 
 my $retry = 1;
-- 
2.15.0.rc1.413.g76aedb451

Re: [RFD] Long term plan with submodule refs?

[Stefan Beller]

On Wed, Nov 8, 2017 at 9:08 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> The relationship is indeed currently useful, but if the long term plan
>>> is to strongly discourage detached submodule HEAD, then I would think
>>> that these patches are in the wrong direction. (If the long term plan is
>>> to end up supporting both detached and linked submodule HEAD, then these
>>> patches are fine, of course.) So I think that the plan referenced in
>>> Junio's email (that you linked above) still needs to be discussed.
>>
>> This email presents different approaches.
>>
>> Objective
>> =
>> This document should summarize the current situation of Git submodules
>> and start a discussion of where it can be headed long term.
>> Show different ways in which submodule refs could evolve.
>>
>> Background
>> ==
>> Submodules in Git are considered as an independet repository currently.
>> This is okay for current workflows, such as utilizing a library that is
>> rarely updated. Other workflows that require a tighter integration between
>> submodule and superproject are possible, but cumbersome as there is an
>> additional step that has to be performed, which is the update of the gitlink
>> pointer in the superproject.
>
> I do not think "rarely updaed" is an issue.
>
> The problem is that we may want to make it easier to use a
> superproject and its submodules as if the combined whole were a
> single project, which currently is not easy, primarily because
> submodules are separate entities with different set of branches that
> can be checked out independently from what branch the superproject
> is working on.

Well and this fact seems to be not a problem in the current use of submodules,
precisely because the workflow either (a) is not too cumbersome or (b)
is executed
not too often to bother enough.

> These are good starting points for copying such a combined whole to
> your local machine and start working on it.  The more interesting,
> important, and potentially difficult part is how the result of such
> work is shared back to where you started from.  "push --recursive"
> may be a simple phrase, but a sensible definition of how it should
> work won't be that simple.
...
>
> We should make detached HEAD safe against gc if it is not,
> regardless of the use of submodules.  I thought it already was made
> safe long time ago.

The detached HEAD itself is protected via its reflog (which is around
for say 2 weeks?)

If I were to develop using detached HEAD only in todays world of
submodules using different branches in the superproject, I run the risk
of loosing some commits in the submodule, as they are not the detached
HEAD all the time, but might even be loose tips.

This combined with the previous paragraph brings in another important
concern:
Some projects would have a very different history when used as a
submodule compared to when used as a stand alone project.
Other projects may be closely aligned between their branches and
what the superproject records.

So the more we deviate from the traditional branch model, the easier
we make it to have the submodule tips be very different from the
standalone tips, which may overexpose us to the gc issues as well as
the general question how much these projects have in common.

>> Use replicate refs in submodules
>> 
>> This approach will replicate the superproject refs into the submodule
>> ref namespace, e.g. git-branch learns about --recurse-submodules, which
>> creates a branch of a given name in all submodules. These (topic) branches
>> should be kept in sync with the superproject
>>
>> Pros:
>>  * This seemed intuitive to Gerrit users
>>  * 'quick' to implement, most of the commands are already there,
>>just git-branch is needed to have the workflows mentioned above complete.
>> Cons:
>>  * What does "git checkout -b A B" mean? (special case: B == HEAD)
>
> The command ran at which level?  In the superproject, or in a single
> submodule?

In the superproject, with --recurse-submodules, as the A and B would recurse
as strings, and not change meaning depending on the gitlink value.

>
>>Is the branch name replicated as a string into the submodule operation,
>>or do we dereference the superprojects gitlink and walk from there?
>
> If they are "kept in sync with the superproject", then there should
> be no difference between the two, so I do not see any room for
> wondering about that.

Except you can still break out by issuing commands in the submodule
to change the submodule refs to be different from the superproject.

This was also more along the lines of thinking about the (Gerrit) remote,
which does and okay, but not stellar job in keeping the remote branches
for superproject and submodule in sync. I'd expect glitches there.

> In other words, if there is need to worry
> about the differences between the above two, then it probably is
> fundamentally impossible to 

Re: [Query] Separate hooks for Git worktrees

[Stefan Beller]

On Thu, Nov 9, 2017 at 2:58 AM, Viresh Kumar  wrote:
> Hi,
>
> I have a typical use case, where I am using the same
> repository for both Android and Linux kernel branches.
>
> Android needs us to keep a special hook "commit-msg"
> which adds a "Change-Id" to every commit we create.

The standard hook for Gerrit can be {en,dis}-abled via
  git config gerrit.createChangeId {true, false}

> While this works fine with Android, the behavior doesn't change
> by simply changing to a upstream kernel branch and eventually
> by mistake I may end up sending patch with Change-Id to upstream
> kernel as well. And I want to avoid that.
>
> I am looking at ways to make this configuration work for me by
> applying the hook only for Android branches.
>
> I tried using the "git worktrees" command to create a separate
> linked tree for my android branch, but it doesn't have a .git directory
> but just a file linking to the main repository.

We have no worktree specific config yet, though patches for
this were floated on the mailing list.

Though recent versions of git learned to conditionally include
config files. (look for includeIf in man git-config), which I think
could be used to set the option gerrit.createChangeId  depending
on the worktree you are in.

> Any idea how I can get around this problem without having separate
> repositories for kernel and android ?

The proposed approach above might be hacky but sounds as if
it should work?

Thanks,
Stefan

Hello Dear,

[Mr Hassan Umra]

Dear,
Please confirm back, can your account receive $7,500.000.00 Million? Reply so 
we can discuss details and what will be your commission. The transaction is 
genuine; I took the decision because of pressure on me I decided to leave my 
country for the safety of my live and son.Remember to send me your Full 
information such as: Receiver's Name___ Address: _ Country: _ Phone Number: _ 
Age_ I.D Card:__

Reply here(  hassan.umr...@gmail.com )

MR HASSAN UMRA
Phone Number +229 64614418

Re: [PATCH v2 1/1] Introduce git add --renormalize .

[Torsten Bögershausen]

[]
> 
> If we had such a term in Documentation/glossary-contents.txt, we
> could even say
> 
>   Add contents of all paths to the index by freshly applying
>   the "clean" process, even to the ones Git may think are
>   unmodified in the working tree since they were added the
>   last time (based on the file timestamps etc.).  This is
>   often useful after updating settings like `core.autocrlf` in
>   the `.git/config` file and the `text` attributes in the
>   `.gitattributes` file to correct the index entries that
>   records lines with CRLF to use LF instead, or changing what
>   the `clean` filter does.  This option implies `-u`.
> 
> The point is to express that the CRLF/LF is a consequence (even
> though it may be the most prominent one from end-users' point of
> view) of a larger processing.

Here is a somwhat shorter description:

Apply the "clean" process freshly to all tracked files.
This is useful after changing `core.autocrlf` or the `text`
attributes in the `.gitattributes` file because
Git may not consider these files as changed.
Correct the files that had been commited with CRLF,
they will from now on have LF instead.
Re-run what the `clean` filter does.
This option implies `-u`.


> 
> > [snip the TC. Adding line endings is good)
> 
> What is TC in this context?

Sorry for confusion: TC means test case.

Re: [PATCH] notes: add `rm` and `delete` commands

[Eric Sunshine]

On Thu, Nov 9, 2017 at 8:46 AM, Adam Dinwoodie  wrote:
> Add `git notes rm` and `git notes delete` as alternative ways of saying
> `git notes remove`.

The justification for this change seems to be missing from the commit message.

One can formulate arguments for the change:

- "rm" & "delete" more intuitive for Unix and DOS users
- for consistency with git- command(s)
- ...

or against the change:

- synonym bloat; balloons documentation
- steals command verbs from potential future features
- ...

> Signed-off-by: Adam Dinwoodie 

Re: [PATCH v3] doc/SubmittingPatches: correct subject guidance

[Eric Sunshine]

On Thu, Nov 9, 2017 at 8:08 AM, Adam Dinwoodie  wrote:
> The examples and common practice for adding markers such as "RFC" or
> "v2" to the subject of patch emails is to have them within the same
> brackets as the "PATCH" text, not after the closing bracket.  Further,
> the practice of `git format-patch` and the like, as well as what appears
> to be the more common pratice on the mailing list, is to use "[RFC
> PATCH]", not "[PATCH/RFC]".
>
> Update the SubmittingPatches article to match, and to reference the
> `format-patch` helper arguments.
>
> Signed-off-by: Adam Dinwoodie 
> ---
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> @@ -184,21 +184,25 @@ lose tabs that way if you are not careful.
>  It is a common convention to prefix your subject line with
>  [PATCH].  This lets people easily distinguish patches from other
> -e-mail discussions.  Use of additional markers after PATCH and
> -the closing bracket to mark the nature of the patch is also
> -encouraged.  E.g. [PATCH/RFC] is often used when the patch is
> +e-mail discussions.  Use of markers in addition to PATCH within
> +the brackets to describe the nature of the patch is also
> +encouraged.  E.g. [RFC PATCH] is often used when the patch is
>  not ready to be applied but it is for discussion, [PATCH v2],

Not a new problem, but since you're here cleaning this up, the "not
ready to be applied but it is for discussion" makes for a clunky read.
Perhaps something roughly like:

E.g. [RFC PATCH] is often used to indicate that a patch needs
further discussion ("request for comments") before being
accepted.

>  [PATCH v3] etc. are often seen when you are sending an update to
>  what you have previously sent.
>
> -"git format-patch" command follows the best current practice to
> +The "git format-patch" command follows the best current practice to
>  format the body of an e-mail message.  At the beginning of the
>  patch should come your commit message, ending with the
>  Signed-off-by: lines, and a line that consists of three dashes,
>  followed by the diffstat information and the patch itself.  If
>  you are forwarding a patch from somebody else, optionally, at
>  the beginning of the e-mail message just before the commit
>  message starts, you can put a "From: " line to name that person.
> +To change the bracketed text at the start of the subject, use
> +`git format-patch --subject-prefix=`.  As a shortcut, you

This may be nit-picky, but it took a bit of thought for me to work out
what "bracketed text at the start of the subject" meant. I wonder if
it would be clearer just to spell it out:

To change the default "[PATCH]" in the subject to "[]", use
`git format-patch --subject-prefix=`.

> +can use `--rfc` instead of `--subject-prefix="RFC PATCH"`, or
> +`-v ` instead of `--subject-prefix="PATCH v"`.

Overall, this is much easier to digest than the run-on sentence in v2. Thanks.

[PATCH] fetch-object: include the fetch-object.h header file

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Jeff,

If you need to re-roll your 'jh/fsck-promisors' branch, could you
please squash this into the relevant patch (commit 1a4b4ca4e9,
"introduce fetch-object: fetch one promisor object", 02-11-2017).

[This silences a sparse warning].

Also, I notice that partial_clone_utils_register() does not have
any callers - I assume this will be 'fixed' in an upcoming series.

Thanks!

ATB,
Ramsay Jones

 fetch-object.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fetch-object.c b/fetch-object.c
index 369b61c0e..fc086fce2 100644
--- a/fetch-object.c
+++ b/fetch-object.c
@@ -3,6 +3,7 @@
 #include "pkt-line.h"
 #include "strbuf.h"
 #include "transport.h"
+#include "fetch-object.h"
 
 void fetch_object(const char *remote_name, const unsigned char *sha1)
 {
-- 
2.15.0

[PATCH v1 4/4] fastindex: add documentation for the fastindex extension

[Ben Peart]

This includes the core.fastindex setting, the update-index additions,
and the fastindex index extension.

Signed-off-by: Ben Peart 
---
 Documentation/config.txt |  8 
 Documentation/git-update-index.txt   | 11 +++
 Documentation/technical/index-format.txt | 26 ++
 3 files changed, 45 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f0d62753d..269ff97c8c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -868,6 +868,14 @@ relatively high IO latencies.  When enabled, Git will do 
the
 index comparison to the filesystem data in parallel, allowing
 overlapping IO's.  Defaults to true.
 
+core.fastIndex::
+   Enable parallel index loading
++
+This can speed up operations like 'git diff' and 'git status' especially
+when the index is very large.  When enabled, Git will do the index
+loading from the on disk format to the in-memory format in parallel.
+Defaults to false.
+
 core.createObject::
You can set this to 'link', in which case a hardlink followed by
a delete of the source are used to make sure that object creation
diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index 75c7dd9dea..7ffd285c94 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -19,6 +19,7 @@ SYNOPSIS
 [--ignore-submodules]
 [--[no-]split-index]
 [--[no-|test-|force-]untracked-cache]
+[--[no-]fastindex]
 [--really-refresh] [--unresolve] [--again | -g]
 [--info-only] [--index-info]
 [-z] [--stdin] [--index-version ]
@@ -201,6 +202,16 @@ will remove the intended effect of the option.
`--untracked-cache` used to imply `--test-untracked-cache` but
this option would enable the extension unconditionally.
 
+--fastindex::
+--no-fastindex::
+   Enable or disable the fast index feature. These options
+   take effect whatever the value of the `core.fastindex`
+   configuration variable (see linkgit:git-config[1]). But a warning
+   is emitted when the change goes against the configured value, as
+   the configured value will take effect next time the index is
+   read and this will remove the intended effect of the option.
+
+
 \--::
Do not interpret any more arguments as options.
 
diff --git a/Documentation/technical/index-format.txt 
b/Documentation/technical/index-format.txt
index ade0b0c445..e37b4cf874 100644
--- a/Documentation/technical/index-format.txt
+++ b/Documentation/technical/index-format.txt
@@ -295,3 +295,29 @@ The remaining data of each directory block is grouped by 
type:
 in the previous ewah bitmap.
 
   - One NUL.
+
+== Index Entry Offset Table
+
+  The Index Entry Offset Table (IEOT) is used to help address the CPU
+  cost of loading the index by enabling multi-threading the process of
+  converting cache entries from the on-disk format to the in-memory format.
+  Because it must be able to be loaded before the variable length cache
+  entries and other index extensions, this extension must be written last.
+  The signature for this extension is { 'I', 'E', 'O', 'T' }.
+
+  The extension consists of:
+
+  - 32-bit version (currently 1)
+
+  - A number of index offset entries each consisting of:
+
+- 32-bit offset from the begining of the file to the first cache entry
+ in this block of entries.
+
+- 32-bit count of cache entries in this block
+
+  - 32-bit version (currently 1)
+
+  - 32-bit size of the extension
+
+  - 4-byte extension signature
-- 
2.15.0.windows.1

[PATCH v1 1/4] fastindex: speed up index load through parallelization

[Ben Peart]

This patch helps address the CPU cost of loading the index by adding
additional data to the index that will allow us to multi-thread the
loading and conversion of cache entries.

It accomplishes this by adding an (optional) index extension that is a
table of offsets to blocks of cache entries in the index file.  With
version 2, 3 or even 4 indexes, we can utilize the Index Entry Offset Table
(IEOT) to parallelize the loading and conversion of the cache entries
across all available CPU cores.

To make this work for V4 indexes, when writing the index, it periodically
"resets" the prefix-compression by encoding the current entry as if the
path name for the previous entry is completely different and saves the
offset of that entry in the IEOT.  Basically, with V4 indexes, it
generates offsets into blocks of prefix-compressed entries.

To enable reading the IEOT extension before reading all the variable
length cache entries and other extensions, the IEOT is written last,
right before the trailing SHA1.

The format of that extension has the signature bytes and size at the
beginning (like a normal extension) as well as at the end in reverse
order to enable parsing the extension by seeking back from the end of
the file.

During index load, read the index header then seek to the end of the
index, back up past the trailing SHA1 and look for the IEOT extension
signature bytes.  If they exist, read the 32-bit size and seek back to
the extension header and verify the leading header and size bits.  If
they all match, we can be assured we have a valid IEOT extension.

If the IEOT extension is available, create multiple threads to divide
the work of loading and converting the cache entries across all
available CPU cores.  Once the cache entries are loaded, the rest of the
extensions can be loaded and processed normally (skipping the IEOT entry
as it has already been processed).  If the IEOT extension is not
available then parsing the index will proceed as usual with a single thread.

Signed-off-by: Ben Peart 
---
 cache.h   |  25 +
 config.c  |  20 
 config.h  |   1 +
 environment.c |   3 +
 read-cache.c  | 343 ++
 5 files changed, 371 insertions(+), 21 deletions(-)

diff --git a/cache.h b/cache.h
index d74f00d8db..ea0c4b4b97 100644
--- a/cache.h
+++ b/cache.h
@@ -794,6 +794,7 @@ extern char *git_replace_ref_base;
 
 extern int fsync_object_files;
 extern int core_preload_index;
+extern int core_fast_index;
 extern int core_apply_sparse_checkout;
 extern int precomposed_unicode;
 extern int protect_hfs;
@@ -1942,4 +1943,28 @@ void sleep_millisec(int millisec);
  */
 void safe_create_dir(const char *dir, int share);
 
+
+#ifndef NO_PTHREADS
+struct index_entry_offset
+{
+   /* starting byte offset into index file, count of index entries in this 
block */
+   int offset, nr;
+};
+
+struct index_entry_offset_table
+{
+   int nr;
+   struct index_entry_offset entries[0];
+};
+
+extern struct index_entry_offset_table *read_ieot_extension(void *mmap, size_t 
mmap_size);
+
+struct ondisk_cache_entry;
+extern struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,
+   unsigned long *ent_size,
+   struct strbuf *previous_name,
+   int use_length);
+
+#endif
+
 #endif /* CACHE_H */
diff --git a/config.c b/config.c
index 903abf9533..44b08f57eb 100644
--- a/config.c
+++ b/config.c
@@ -1203,6 +1203,11 @@ static int git_default_core_config(const char *var, 
const char *value)
return 0;
}
 
+   if (!strcmp(var, "core.fastindex")) {
+   core_fast_index = git_config_bool(var, value);
+   return 0;
+   }
+
if (!strcmp(var, "core.createobject")) {
if (!strcmp(value, "rename"))
object_creation_mode = OBJECT_CREATION_USES_RENAMES;
@@ -2156,6 +2161,21 @@ int git_config_get_max_percent_split_change(void)
return -1; /* default value */
 }
 
+int ignore_fast_index_config;
+int git_config_get_fast_index(void)
+{
+   int val;
+
+   /* Hack for test programs like test-ieot */
+   if (ignore_fast_index_config)
+   return core_fast_index;
+
+   if (!git_config_get_maybe_bool("core.fastindex", ))
+   return val;
+
+   return -1; /* default value */
+}
+
 NORETURN
 void git_die_config_linenr(const char *key, const char *filename, int linenr)
 {
diff --git a/config.h b/config.h
index a49d264416..3f78b1d262 100644
--- a/config.h
+++ b/config.h
@@ -212,6 +212,7 @@ extern int git_config_get_pathname(const char *key, const 
char **dest);
 extern int git_config_get_untracked_cache(void);
 extern int git_config_get_split_index(void);
 extern int git_config_get_max_percent_split_change(void);
+extern int git_config_get_fast_index(void);
 
 /* This dies if the configured or default date is in the future */
 extern int git_config_get_expiry(const char *key, const char 

[PATCH v1 2/4] update-index: add fastindex support to update-index

[Ben Peart]

Add support in update-index to manually add/remove the fastindex
extension via --[no-]fastindex flags.

Signed-off-by: Ben Peart 
---
 builtin/update-index.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index fefbe60167..63b7f18807 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -917,6 +917,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
struct refresh_params refresh_args = {0, _errors};
int lock_error = 0;
int split_index = -1;
+   int fast_index = -1;
struct lock_file lock_file = LOCK_INIT;
struct parse_opt_ctx_t ctx;
strbuf_getline_fn getline_fn;
@@ -1008,6 +1009,8 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
N_("test if the filesystem supports untracked 
cache"), UC_TEST),
OPT_SET_INT(0, "force-untracked-cache", _cache,
N_("enable untracked cache without testing the 
filesystem"), UC_FORCE),
+   OPT_BOOL(0, "fastindex", _index,
+   N_("enable or disable fast index parsing")),
OPT_END()
};
 
@@ -1146,6 +1149,25 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
die("BUG: bad untracked_cache value: %d", untracked_cache);
}
 
+   if (fast_index > 0) {
+   if (git_config_get_fast_index() == 0)
+   warning(_("core.fastindex is unset; "
+   "set it if you really want to "
+   "enable fastindex"));
+   core_fast_index = 1;
+   active_cache_changed |= SOMETHING_CHANGED;
+   report(_("fastindex enabled"));
+   }
+   else if (!fast_index) {
+   if (git_config_get_fast_index() == 1)
+   warning(_("core.fastindex is set; "
+   "remove it if you really want to "
+   "disable fastindex"));
+   core_fast_index = 0;
+   active_cache_changed |= SOMETHING_CHANGED;
+   report(_("fastindex disabled"));
+   }
+
if (active_cache_changed) {
if (newfd < 0) {
if (refresh_args.flags & REFRESH_QUIET)
-- 
2.15.0.windows.1

[PATCH v1 3/4] fastindex: add test tools and a test script

[Ben Peart]

Add a test utility (test-dump-fast-index) that dumps the contents of the
IEOT to stdout.  This enables checking the existance of the extension
and the ability to parse and output its contents.

Add a test utility (test-fast-index) that loads the index using the
fastindex logic then loads it using the regular logic and compares the
results of the two.  Any differences are reported and an error is returned.

Add a test script (t1800-fast-index.sh) to:

Test the ability to add/remove the fastindex index extension via
update-index.

Verify that loading the index with and without the fastindex extension
return the same results with both V2 and V4 indexes.

Signed-off-by: Ben Peart 
---
 Makefile|  2 +
 t/helper/test-dump-fast-index.c | 68 +
 t/helper/test-fast-index.c  | 84 +
 t/t1800-fast-index.sh   | 55 +++
 4 files changed, 209 insertions(+)
 create mode 100644 t/helper/test-dump-fast-index.c
 create mode 100644 t/helper/test-fast-index.c
 create mode 100644 t/t1800-fast-index.sh

diff --git a/Makefile b/Makefile
index cd75985991..a7df82721e 100644
--- a/Makefile
+++ b/Makefile
@@ -647,9 +647,11 @@ TEST_PROGRAMS_NEED_X += test-config
 TEST_PROGRAMS_NEED_X += test-date
 TEST_PROGRAMS_NEED_X += test-delta
 TEST_PROGRAMS_NEED_X += test-dump-cache-tree
+TEST_PROGRAMS_NEED_X += test-dump-fast-index
 TEST_PROGRAMS_NEED_X += test-dump-split-index
 TEST_PROGRAMS_NEED_X += test-dump-untracked-cache
 TEST_PROGRAMS_NEED_X += test-fake-ssh
+TEST_PROGRAMS_NEED_X += test-fast-index
 TEST_PROGRAMS_NEED_X += test-genrandom
 TEST_PROGRAMS_NEED_X += test-hashmap
 TEST_PROGRAMS_NEED_X += test-index-version
diff --git a/t/helper/test-dump-fast-index.c b/t/helper/test-dump-fast-index.c
new file mode 100644
index 00..4e6d28d660
--- /dev/null
+++ b/t/helper/test-dump-fast-index.c
@@ -0,0 +1,68 @@
+#include "cache.h"
+
+int cmd_main(int ac, const char **av)
+{
+#ifndef NO_PTHREADS
+   const char *path;
+   int fd, i;
+   struct stat st;
+   void *mmap;
+   size_t mmap_size;
+   struct cache_header *hdr;
+   struct index_entry_offset_table *ieot;
+   struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
+   int err = 0;
+
+   setup_git_directory();
+   path = get_index_file();
+   fd = open(path, O_RDONLY);
+   if (fd < 0) {
+   die("%s: index file open failed", path);
+   }
+
+   if (fstat(fd, ))
+   die("cannot stat the open index");
+
+   mmap_size = xsize_t(st.st_size);
+   if (mmap_size < sizeof(struct cache_header) + GIT_SHA1_RAWSZ)
+   die("index file smaller than expected");
+
+   mmap = xmmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);
+   if (mmap == MAP_FAILED)
+   die("unable to map index file");
+   close(fd);
+
+   hdr = mmap;
+   if (ntohl(hdr->hdr_version) == 4)
+   previous_name = _name_buf;
+   else
+   previous_name = NULL;
+
+   ieot = read_ieot_extension(mmap, mmap_size);
+   if (ieot) {
+   printf("IEOT with %d entries\n", ieot->nr);
+   printf("  OffsetCount Name\n");
+   printf("  \n");
+   for (i = 0; i < ieot->nr; i++) {
+   struct ondisk_cache_entry *disk_ce;
+   struct cache_entry *ce;
+   unsigned long consumed;
+
+   disk_ce = (struct ondisk_cache_entry *)((char *)mmap + 
ieot->entries[i].offset);
+   ce = create_from_disk(disk_ce, , 
previous_name, 0);
+   printf("%8d %8d %.*s\n", ieot->entries[i].offset, 
ieot->entries[i].nr, ce->ce_namelen, ce->name);
+   free(ce);
+   }
+   } else {
+   printf("missing or invalid extension");
+   err = 1;
+   }
+
+   free(ieot);
+   munmap(mmap, mmap_size);
+   return err;
+#else
+   die("ieot only supported with PTHREADS");
+   return -1;
+#endif
+}
diff --git a/t/helper/test-fast-index.c b/t/helper/test-fast-index.c
new file mode 100644
index 00..fe63130ba0
--- /dev/null
+++ b/t/helper/test-fast-index.c
@@ -0,0 +1,84 @@
+#include "cache.h"
+
+int compare_ce(const struct cache_entry *ce1, const struct cache_entry *ce2)
+{
+   /*  struct hashmap_entry ent; */
+   /*  struct stat_data ce_stat_data; */
+
+   if (ce1->ce_mode != ce2->ce_mode) {
+   printf("ce_mode: %d:%d\n", ce1->ce_mode, ce2->ce_mode);
+   return 1;
+   }
+
+   if (ce1->ce_flags != ce2->ce_flags) {
+   printf("ce_flags: %d:%d\n", ce1->ce_flags, ce2->ce_flags);
+   return 1;
+   }
+
+   if (ce1->ce_namelen != ce2->ce_namelen) {
+   printf("namelen: %d:%d\n", 

[PATCH v1 0/4] Speed up index load through parallelization

[Ben Peart]

This patch will help address the CPU cost of loading the index by adding
a table of contents extension to the index that will allow us to
multi-thread the loading and conversion of cache entries from the on-disk
format, to the in-memory format.  This is particularly beneficial
with large indexes and V4 indexes which have more CPU cost due to the
prefix-encoding.

I wanted to get feedback on the concept as the way I'm adding the table
of contents information via an extension that can be read before the
variable length section of cache entries and other extensions is a bit
of a clever hack (see below) as is the resetting of the prefix encoding
for V4 indexes. Both, however, are entirely backwards compatible with
older versions of git which can still properly read and use the index.

I'm not particularly fond of the names "fastindex" and "IEOT." I've
wondered if "indextoc" and "Index Table of Contents (ITOC)" would be
better names but I'm open to suggestions.

As there is overhead to spinning up a thread, there is logic to only
do the index loading in parallel when there are enough entries for it
to help (currently set at 7,500 per thread with a minimum of 2 threads).

The impact of the change can be seen using t/helper/test-read-cache:

fastindex
testcount   files   TRUEFALSE Savings

test-read-cache 500 100K6.398.33  23.36%
test-read-cache 100 1M  12.49   18.68 33.12%

The on-disk format looks like this:

Index header
Cache entry 1
Cache entry 2
.
.
.
Extension 1
Extension 2
.
.
Index Entry Offset Table Extension (must be written last!)
IEOT signature bytes
32-bit size
32-bit version
32-bit Cache Entry Offset 1
32-bit Cache Entry count
32-bit Cache Entry Offset 2
32-bit Cache Entry count
.
.
.
32-bit version
32-bit size
IEOT signature bytes
SHA1

Signed-off-by: Ben Peart 

Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/1146d38932
Checkout: git fetch https://github.com/benpeart/git fastindex-v1 && git 
checkout 1146d38932

Ben Peart (4):
  fastindex: speed up index load through parallelization
  update-index: add fastindex support to update-index
  fastindex: add test tools and a test script
  fastindex: add documentation for the fastindex extension

 Documentation/config.txt |   8 +
 Documentation/git-update-index.txt   |  11 +
 Documentation/technical/index-format.txt |  26 +++
 Makefile |   2 +
 builtin/update-index.c   |  22 ++
 cache.h  |  25 +++
 config.c |  20 ++
 config.h |   1 +
 environment.c|   3 +
 read-cache.c | 343 +--
 t/helper/test-dump-fast-index.c  |  68 ++
 t/helper/test-fast-index.c   |  84 
 t/t1800-fast-index.sh|  55 +
 13 files changed, 647 insertions(+), 21 deletions(-)
 create mode 100644 t/helper/test-dump-fast-index.c
 create mode 100644 t/helper/test-fast-index.c
 create mode 100644 t/t1800-fast-index.sh


base-commit: 7668cbc60578f99a4c048f8f8f38787930b8147b
-- 
2.15.0.windows.1

[PATCH] notes: add `rm` and `delete` commands

[Adam Dinwoodie]

Add `git notes rm` and `git notes delete` as alternative ways of saying
`git notes remove`.

Signed-off-by: Adam Dinwoodie 
---
 Documentation/git-notes.txt | 4 +++-
 builtin/notes.c | 8 +---
 t/t3301-notes.sh| 6 +++---
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 43677297f..b1059dc85 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -17,7 +17,7 @@ SYNOPSIS
 'git notes' merge [-v | -q] [-s  ] 
 'git notes' merge --commit [-v | -q]
 'git notes' merge --abort [-v | -q]
-'git notes' remove [--ignore-missing] [--stdin] [...]
+'git notes' (remove|rm|delete) [--ignore-missing] [--stdin] [...]
 'git notes' prune [-n | -v]
 'git notes' get-ref
 
@@ -110,6 +110,8 @@ When done, the user can either finalize the merge with
 'git notes merge --abort'.
 
 remove::
+rm::
+delete::
Remove the notes for given objects (defaults to HEAD). When
giving zero or one object from the command line, this is
equivalent to specifying an empty note message to
diff --git a/builtin/notes.c b/builtin/notes.c
index 12afdf190..cedb37535 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -32,7 +32,7 @@ static const char * const git_notes_usage[] = {
N_("git notes [--ref ] merge [-v | -q] [-s ] 
"),
N_("git notes merge --commit [-v | -q]"),
N_("git notes merge --abort [-v | -q]"),
-   N_("git notes [--ref ] remove [...]"),
+   N_("git notes [--ref ] (remove|rm|delete) [...]"),
N_("git notes [--ref ] prune [-n | -v]"),
N_("git notes [--ref ] get-ref"),
NULL
@@ -77,7 +77,7 @@ static const char * const git_notes_merge_usage[] = {
 };
 
 static const char * const git_notes_remove_usage[] = {
-   N_("git notes remove []"),
+   N_("git notes (remove|rm|delete) []"),
NULL
 };
 
@@ -1012,7 +1012,9 @@ int cmd_notes(int argc, const char **argv, const char 
*prefix)
result = show(argc, argv, prefix);
else if (!strcmp(argv[0], "merge"))
result = merge(argc, argv, prefix);
-   else if (!strcmp(argv[0], "remove"))
+   else if (!strcmp(argv[0], "remove") ||
+!strcmp(argv[0], "rm") ||
+!strcmp(argv[0], "delete"))
result = remove_cmd(argc, argv, prefix);
else if (!strcmp(argv[0], "prune"))
result = prune(argc, argv, prefix);
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 2d200fdf3..50df9a3f9 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -353,9 +353,9 @@ test_expect_success 'create note with combination of -m and 
-F' '
test_cmp expect-combine_m_and_F actual
 '
 
-test_expect_success 'remove note with "git notes remove"' '
+test_expect_success 'remove note with "git notes remove/rm"' '
git notes remove HEAD^ &&
-   git notes remove &&
+   git notes rm &&
cat >expect-rm-remove <<-EOF &&
commit 7f9ad8836c775acb134c0a055fc55fb4cd1ba361
Author: A U Thor 
@@ -390,7 +390,7 @@ test_expect_success 'removing more than one' '
# We have only two -- add another and make sure it stays
git notes add -m "extra" &&
git notes list HEAD >after-removal-expect &&
-   git notes remove HEAD^^ HEAD^^^ &&
+   git notes delete HEAD^^ HEAD^^^ &&
git notes list | sed -e "s/ .*//" >actual &&
test_cmp after-removal-expect actual
 '
-- 
2.14.3

Re: Bug - Status - Space in Filename

[Jeff King]

On Thu, Nov 09, 2017 at 12:26:20PM +0900, Junio C Hamano wrote:

> The difference in d->head_path part is dealing about renames, which
> is irrelevant for showing unmerged paths, but the key difference is
> that the _unmerged() forgets to add the enclosing "" around the
> result of quote_path().
> 
> It seems that wt_shortstatus_other() shares the same issue.  Perhaps
> this will "fix" it?
> 
> Having said all that, the whole "quote path using c-style" was
> designed very deliberately to treat SP (but not other kind of
> whitespaces like HT) as something we do *not* have to quote (and
> more importantly, do not *want* to quote, as pathnames with SP in it
> are quite common for those who are used to "My Documents/" etc.), so
> it is arguably that what is broken and incorrect is the extra
> quoting with dq done in wt_shortstatus_status().  It of course is
> too late to change the rules this late in the game, though.

Yeah, I think the original sin is using " -> " in the --porcelain output
(which just got it from --short). That should have been a HT all along
to match the rest of the diff code. The --porcelain=v2 format gets this
right.

> Perhaps a better approach is to refactor the extra quoting I moved
> to this emit_with_optional_dq() helper into quote_path() which
> currently is just aliased to quote_path_relative().  It also is
> possible that we may want to extend the "no_dq" parameter that is
> given to quote_c_style() helper from a boolean to a set of flag
> bits, and allow callers to request "I want SP added to the set of
> bytes that triggers the quoting".

Yeah, I had the same thought upon digging into the code.

That said, if this is the only place that has this funny quoting, it may
not be worth polluting the rest of the code with the idea that quoting
spaces is a good thing to do.

> diff --git a/wt-status.c b/wt-status.c
> index bedef256ce..472d53d596 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1671,6 +1671,20 @@ static void wt_longstatus_print(struct wt_status *s)
>   wt_longstatus_print_stash_summary(s);
>  }
>  
> +static const char *emit_with_optional_dq(const char *in, const char *prefix, 
> +   struct strbuf *out)
> +{
> + const char *one;
> +
> + one = quote_path(in, prefix, out);
> + if (*one != '"' && strchr(one, ' ') != NULL) {
> + putchar('"');
> + strbuf_addch(out, '"');
> + one = out->buf;
> + }
> + return one;
> +}

This puts part of its output to stdout, and the other part into a
strbuf. That works for these callers, but maybe just emitting the whole
thing to stdout would be less confusing (and I suspect would clean up
the callers a bit, who would not have to worry about the strbuf at all).

-Peff

[PATCH v3] doc/SubmittingPatches: correct subject guidance

[Adam Dinwoodie]

The examples and common practice for adding markers such as "RFC" or
"v2" to the subject of patch emails is to have them within the same
brackets as the "PATCH" text, not after the closing bracket.  Further,
the practice of `git format-patch` and the like, as well as what appears
to be the more common pratice on the mailing list, is to use "[RFC
PATCH]", not "[PATCH/RFC]".

Update the SubmittingPatches article to match, and to reference the
`format-patch` helper arguments.

Signed-off-by: Adam Dinwoodie 
Helped-by: Eric Sunshine 
---
 Documentation/SubmittingPatches | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 558d465b6..ae59fd9d0 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -184,21 +184,25 @@ lose tabs that way if you are not careful.
 
 It is a common convention to prefix your subject line with
 [PATCH].  This lets people easily distinguish patches from other
-e-mail discussions.  Use of additional markers after PATCH and
-the closing bracket to mark the nature of the patch is also
-encouraged.  E.g. [PATCH/RFC] is often used when the patch is
+e-mail discussions.  Use of markers in addition to PATCH within
+the brackets to describe the nature of the patch is also
+encouraged.  E.g. [RFC PATCH] is often used when the patch is
 not ready to be applied but it is for discussion, [PATCH v2],
 [PATCH v3] etc. are often seen when you are sending an update to
 what you have previously sent.
 
-"git format-patch" command follows the best current practice to
+The "git format-patch" command follows the best current practice to
 format the body of an e-mail message.  At the beginning of the
 patch should come your commit message, ending with the
 Signed-off-by: lines, and a line that consists of three dashes,
 followed by the diffstat information and the patch itself.  If
 you are forwarding a patch from somebody else, optionally, at
 the beginning of the e-mail message just before the commit
 message starts, you can put a "From: " line to name that person.
+To change the bracketed text at the start of the subject, use
+`git format-patch --subject-prefix=`.  As a shortcut, you
+can use `--rfc` instead of `--subject-prefix="RFC PATCH"`, or
+`-v ` instead of `--subject-prefix="PATCH v"`.
 
 You often want to add additional explanation about the patch,
 other than the commit message itself.  Place such "cover letter"
-- 
2.14.3

Re: [PATCH v2] doc/SubmittingPatches: correct subject guidance

[Adam Dinwoodie]

On Wednesday 08 November 2017 at 09:10 am -0500, Eric Sunshine wrote:
> On Wed, Nov 8, 2017 at 8:47 AM, Adam Dinwoodie  wrote:
> > +e-mail discussions.  Use of markers in addition to PATCH within
> > +the brackets to describe the nature of the patch is also
> > +encouraged.  E.g. [RFC PATCH] is often used when the patch is not
> > +ready to be applied but it is for discussion, and can be added
> > +with the `--rfc` argument to `git format-patch` or `git
> > +send-email`, while [PATCH v2], [PATCH v3] etc.  are often seen
> 
> It has become a bit of a run-on sentence, but aside from that and the
> unnecessary extra whitespace between "etc." and "are", it looks good
> to me.

Both good points, thank you!  I suspect the extra whitespace was a
result of Vim being "helpful" when reflowing the text.

I'll re-spin now with fixed whitespace and breaking up the sentence a
bit.

[PATCH] Documentation: fix typos for the reference in new-command.txt

[Kazuo Yagi]

---
 Documentation/howto/new-command.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/howto/new-command.txt 
b/Documentation/howto/new-command.txt
index 15a4c8031f1f3..ac73c98be72de 100644
--- a/Documentation/howto/new-command.txt
+++ b/Documentation/howto/new-command.txt
@@ -1,13 +1,13 @@
 From: Eric S. Raymond 
 Abstract: This is how-to documentation for people who want to add extension
- commands to Git.  It should be read alongside api-builtin.txt.
+ commands to Git.  It should be read alongside builtin.h.
 Content-type: text/asciidoc
 
 How to integrate new subcommands
 
 
 This is how-to documentation for people who want to add extension
-commands to Git.  It should be read alongside api-builtin.txt.
+commands to Git.  It should be read alongside builtin.h.
 
 Runtime environment
 ---
@@ -48,7 +48,7 @@ binary); this organization makes it easy for people reading 
the code
 to find things.
 
 See the CodingGuidelines document for other guidance on what we consider
-good practice in C and shell, and api-builtin.txt for the support
+good practice in C and shell, and builtin.h for the support
 functions available to built-in commands written in C.
 
 What every extension command needs

--
https://github.com/git/git/pull/430

[Query] Separate hooks for Git worktrees

[Viresh Kumar]

Hi,

I have a typical use case, where I am using the same
repository for both Android and Linux kernel branches.

Android needs us to keep a special hook "commit-msg"
which adds a "Change-Id" to every commit we create.

While this works fine with Android, the behavior doesn't change
by simply changing to a upstream kernel branch and eventually
by mistake I may end up sending patch with Change-Id to upstream
kernel as well. And I want to avoid that.

I am looking at ways to make this configuration work for me by
applying the hook only for Android branches.

I tried using the "git worktrees" command to create a separate
linked tree for my android branch, but it doesn't have a .git directory
but just a file linking to the main repository.

Any idea how I can get around this problem without having separate
repositories for kernel and android ?

Thanks in advance.

--
viresh

Re: [PATCH 1/2] quote-email populates the fields

[Nathan PAYRE]

I Will send the modification in the next patch, I prefer to refractor
a part of the code before.

>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 2208dcc21..665c47d15 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -57,6 +57,7 @@ git send-email --dump-aliases
>>  --[no-]bcc* Email Bcc:
>>  --subject * Email "Subject:"
>>  --in-reply-to * Email "In-Reply-To:"
>> +--quote-email* Populate header fields appropriately.

> Likewise.  If what's "appropriate" is clear to the readers, the word
> in this description adds no value because everybody would know how
> fields are populated.  Otherwise, it does not add any value because
> everybody would have no clue how fields are populated.

Remove "approprietly" done.


>> @@ -652,6 +654,70 @@ if (@files) {
>>   usage();
>>  }
>>
>> +if ($quote_email) {
>> + my $error = validate_patch($quote_email);
>> + die "fatal: $quote_email: $error\nwarning: no patches were sent\n"
>> + if $error;

> validate_patch() calls sendemail-validate hook that is expecting to
> be fed a patch email you are going to send out that might have
> errors so that it can catch it and save you from embarrassment.  The
> file you are feeding it is *NOT* what you are going to send out, but
> is what you are responding to with your patch.  Even if it had an
> embarassing error as a patch, that is not something you care about
> (and it is something you received, so catching this late won't save
>  the sender from embarrassment anyway).

I will remove lines which use validate_patch().


>> + chomp($header[$#header]);
>> + s/^\s+/ /;
>> + $header[$#header] .= $_;
>> + } else {
>> + push(@header, $_);
>> + }
>> + }

> You do not use $fh after this point.  Do not force readers to
> realize that fact by scanning to the end of the function--instead,
> close it here.

In fact $fh is reuse at the end of the if($quote_email) {} but if you
don't see it maybe it's because it's anormal to reuse it after a
long block of code, that's why I think to create a subroutine
for the following code which is similar to the part of if($compose).

foreach (@header) {
   my $initial_sender = $sender || $repoauthor || $repocommitter || '';

   chomp;

   if (/^Subject:\s+(.*)$/i) {
  my $prefix_re = "";
  my $subject_re = $1;
  if ($1 =~ /^[^Re:]/) {
 $prefix_re = "Re: ";
  }
  $initial_subject = $prefix_re . $subject_re;
   } elsif (/^From:\s+(.*)$/i) {
  $recipient = $1;
  push @initial_to, $recipient;
   } elsif (/^To:\s+(.*)$/i) {
  foreach my $addr (parse_address_line($1)) {
 if (!($addr eq $initial_sender)) {
push @initial_cc, $addr;
 }
  }
   } elsif (/^Cc:\s+(.*)$/i) {
  foreach my $addr (parse_address_line($1)) {
 my $qaddr = unquote_rfc2047($addr);
 my $saddr = sanitize_address($qaddr);
 if ($saddr eq $initial_sender) {
next if ($suppress_cc{'self'});
 } else {
next if ($suppress_cc{'cc'});
 }
 push @initial_cc, $addr;
  }
   } elsif (/^Message-Id: (.*)/i) {
  $initial_reply_to = $1;
   } elsif (/^References:\s+(.*)/i) {
  $initial_references = $1;
   } elsif (/^Date: (.*)/i) {
   $date = $1;
   }
}


I close $fh after the second call then.


>> + # Parse the header
>> + foreach (@header) {
>> + my $initial_sender = $sender || $repoauthor || $repocommitter 
>> || '';
>> +
>> + chomp;
>> +
>> + if (/^Subject:\s+(.*)$/i) {
>> + my $prefix_re = "";
>> + my $subject_re = $1;

> What does "_re" mean in the variable name $subject_re?

"_re" mean regular expression but maybe it's clumsy because
it contain the result of a regular expression. What do you think
about rename it into "$prefix" and "$subject" ?


2017-11-01 3:44 GMT+01:00 Junio C Hamano :
> Payre Nathan  writes:
>
>> From: Tom Russello 
>>
>> ---
>
> Missing something here???
>
>>  Documentation/git-send-email.txt |   3 +
>>  git-send-email.perl  |  70 ++-
>>  t/t9001-send-email.sh| 117 
>> +--
>>  3 files changed, 147 insertions(+), 43 deletions(-)
>>
>> diff --git a/Documentation/git-send-email.txt 
>> b/Documentation/git-send-email.txt
>> index bac9014ac..710b5ff32 100644
>> --- a/Documentation/git-send-email.txt
>> +++ b/Documentation/git-send-email.txt
>> @@ -106,6 +106,9 @@ illustration below where `[PATCH v2 0/3]` is in reply to 
>> `[PATCH 0/2]`:
>>  Only necessary if --compose is also set.  If --compose
>>  is not set, this will be prompted for.
>>
>> +--quote-email=::
>> + Fill appropriately header fields for the reply