date:20171027

Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])

The best identification of a blob hash is done via a its path at a
given commit, which this implements.

[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob

Signed-off-by: Stefan Beller 
---
 builtin/describe.c | 58 ++
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 29075dbd0f..752de5843b 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -11,8 +11,9 @@
 #include "hashmap.h"
 #include "argv-array.h"
 #include "run-command.h"
+#include "revision.h"
+#include "list-objects.h"
 
-#define SEEN   (1u << 0)
 #define MAX_TAGS   (FLAG_BITS - 1)
 
 static const char * const describe_usage[] = {
@@ -282,6 +283,50 @@ static void show_suffix(int depth, const struct object_id 
*oid)
printf("-%d-g%s", depth, find_unique_abbrev(oid->hash, abbrev));
 }
 
+struct blob_descriptor {
+   struct object_id current_commit;
+   struct object_id looking_for;
+};
+
+static void process_commit(struct commit *commit, void *data)
+{
+   struct blob_descriptor *bd = data;
+
+   bd->current_commit = commit->object.oid;
+}
+
+static void process_object(struct object *obj, const char *name, void *data)
+{
+   struct blob_descriptor *bd = data;
+
+   if (!oidcmp(>looking_for, >oid))
+   printf(_("blob %s present at path %s in commit %s\n"),
+   oid_to_hex(>looking_for), name,
+   oid_to_hex(>current_commit));
+}
+
+static void describe_blob(struct object_id oid)
+{
+   struct rev_info revs;
+   struct argv_array args = ARGV_ARRAY_INIT;
+   struct blob_descriptor bd = { null_oid, oid };
+
+   argv_array_pushl(, "internal: The first arg is not parsed",
+   "--all", "--single-worktree", "--objects", NULL);
+
+   revs.tree_blobs_in_commit_order = 1;
+
+   init_revisions(, NULL);
+
+   if (setup_revisions(args.argc, args.argv, , NULL) > 1)
+   BUG("setup_revisions could not handle all args?");
+
+   if (prepare_revision_walk())
+   die("revision walk setup failed");
+
+   traverse_commit_list(, process_commit, process_object, );
+}
+
 static void describe(const char *arg, int last_one)
 {
struct object_id oid;
@@ -295,9 +340,14 @@ static void describe(const char *arg, int last_one)
 
if (get_oid(arg, ))
die(_("Not a valid object name %s"), arg);
-   cmit = lookup_commit_reference();
-   if (!cmit)
-   die(_("%s is not a valid '%s' object"), arg, commit_type);
+   cmit = lookup_commit_reference_gently(, 1);
+   if (!cmit) {
+   if (lookup_blob())
+   describe_blob(oid);
+   else
+   die(_("%s is not a commit nor blob"), arg);
+   return;
+   }
 
n = find_commit_name(>object.oid);
if (n && (tags || all || n->prio == 2)) {
-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCH 1/3] list-objects.c: factor out traverse_trees_and_blobs

With traverse_trees_and_blobs factored out of the main traverse function,
the next patch can introduce an in-order revision walking with ease.

Signed-off-by: Stefan Beller 
---
 list-objects.c | 46 --
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/list-objects.c b/list-objects.c
index b3931fa434..0ee0551604 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -183,25 +183,13 @@ static void add_pending_tree(struct rev_info *revs, 
struct tree *tree)
add_pending_object(revs, >object, "");
 }
 
-void traverse_commit_list(struct rev_info *revs,
- show_commit_fn show_commit,
- show_object_fn show_object,
- void *data)
+static void traverse_trees_and_blobs(struct rev_info *revs,
+struct strbuf *base,
+show_object_fn show_object,
+void *data)
 {
int i;
-   struct commit *commit;
-   struct strbuf base;
 
-   strbuf_init(, PATH_MAX);
-   while ((commit = get_revision(revs)) != NULL) {
-   /*
-* an uninteresting boundary commit may not have its tree
-* parsed yet, but we are not going to show them anyway
-*/
-   if (commit->tree)
-   add_pending_tree(revs, commit->tree);
-   show_commit(commit, data);
-   }
for (i = 0; i < revs->pending.nr; i++) {
struct object_array_entry *pending = revs->pending.objects + i;
struct object *obj = pending->item;
@@ -218,17 +206,39 @@ void traverse_commit_list(struct rev_info *revs,
path = "";
if (obj->type == OBJ_TREE) {
process_tree(revs, (struct tree *)obj, show_object,
-, path, data);
+base, path, data);
continue;
}
if (obj->type == OBJ_BLOB) {
process_blob(revs, (struct blob *)obj, show_object,
-, path, data);
+base, path, data);
continue;
}
die("unknown pending object %s (%s)",
oid_to_hex(>oid), name);
}
object_array_clear(>pending);
+}
+
+void traverse_commit_list(struct rev_info *revs,
+ show_commit_fn show_commit,
+ show_object_fn show_object,
+ void *data)
+{
+   struct commit *commit;
+   struct strbuf base;
+   strbuf_init(, PATH_MAX);
+
+   while ((commit = get_revision(revs)) != NULL) {
+   /*
+* an uninteresting boundary commit may not have its tree
+* parsed yet, but we are not going to show them anyway
+*/
+   if (commit->tree)
+   add_pending_tree(revs, commit->tree);
+   show_commit(commit, data);
+   }
+   traverse_trees_and_blobs(revs, , show_object, data);
+
strbuf_release();
 }
-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCH 2/3] revision.h: introduce blob/tree walking in order of the commits

This will be useful shortly.

Signed-off-by: Stefan Beller 
---
 list-objects.c | 2 ++
 revision.h | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/list-objects.c b/list-objects.c
index 0ee0551604..ef64a237d3 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -237,6 +237,8 @@ void traverse_commit_list(struct rev_info *revs,
if (commit->tree)
add_pending_tree(revs, commit->tree);
show_commit(commit, data);
+   if (revs->tree_blobs_in_commit_order)
+   traverse_trees_and_blobs(revs, , show_object, 
data);
}
traverse_trees_and_blobs(revs, , show_object, data);
 
diff --git a/revision.h b/revision.h
index 54761200ad..86985d68aa 100644
--- a/revision.h
+++ b/revision.h
@@ -121,7 +121,8 @@ struct rev_info {
bisect:1,
ancestry_path:1,
first_parent_only:1,
-   line_level_traverse:1;
+   line_level_traverse:1,
+   tree_blobs_in_commit_order:1;
 
/* Diff flags */
unsigned intdiff:1,
-- 
2.15.0.rc2.443.gfcc3b81c0a

[RFC PATCH 0/3] git-describe ?

Occasionally a user is given an object hash from a blob as an error message
or other output (e.g. [1]).

It would be useful to get a further description of such a blob, such as
the (commit, path) tuple where this blob was introduced.

This implements the answer in builtin/describe, but I am not sure if that
is the right place. (One office mate argued it could be a "reverse-blame"
that tells you all the trees/commits where the blob is referenced from).

This is RFC for other reasons as well: tests, docs missing.

Any feedback welcome,

thanks,
Stefan

[1] 
https://stackoverflow.com/questions/10622179/how-to-find-identify-large-files-commits-in-git-history

Stefan Beller (3):
  list-objects.c: factor out traverse_trees_and_blobs
  revision.h: introduce blob/tree walking in order of the commits
  builtin/describe: describe blobs

 builtin/describe.c | 58 ++
 list-objects.c | 48 +++-
 revision.h |  3 ++-
 3 files changed, 86 insertions(+), 23 deletions(-)

-- 
2.15.0.rc2.443.gfcc3b81c0a

RE

2017-10-27 Thread Mrs Alice Walton

I have a charity proposal for you

[PATCH] rebase: exec leaks GIT_DIR to environment

2017-10-27 Thread Jacob Keller

From: Jacob Keller 

I noticed a failure with git rebase interactive mode which causes "exec"
commands to be run with GIT_DIR set. When GIT_DIR is in the environment,
then any command which results in running a git command in
a subdirectory will fail because GIT_DIR=".git".

This unfortunately breaks one of my project's Makefiles, which uses
git-describe to find the version information, but does so from within
a sub directory.

I'm in the process of running a bisect to find where this got
introduced, but I suspect it's part of the rebase--helper changes that
happened a while ago.

Signed-off-by: Jacob Keller 
---
 t/t3404-rebase-interactive.sh | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3704dbb2ecf6..60ab5136f702 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -108,6 +108,17 @@ test_expect_success 'rebase -i with the exec command runs 
from tree root' '
rm -fr subdir
 '
 
+test_expect_failure 'rebase -i with the exec git commands in subdirs still 
work' '
+   test_when_finished "rm -ff subdir" &&
+   test_when_finished "git rebase --abort" &&
+   git checkout master &&
+   mkdir subdir && (cd subdir &&
+   set_fake_editor &&
+   FAKE_LINES="1 exec_>cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
+   git rebase -i HEAD^
+   )
+'
+
 test_expect_success 'rebase -i with the exec command checks tree cleanness' '
git checkout master &&
set_fake_editor &&
-- 
2.11.1.4.gad8c7cd

[PATCH v3 1/4] fsmonitor: Set the PWD to the top of the working tree

The fsmonitor command inherits the PWD of its caller, which may be
anywhere in the working copy.  This makes is difficult for the
fsmonitor command to operate on the whole repository.  Specifically,
for the watchman integration, this causes each subdirectory to get its
own watch entry.

Set the CWD to the top of the working directory, for consistency.

Signed-off-by: Alex Vandiver 
---
 fsmonitor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fsmonitor.c b/fsmonitor.c
index 7c1540c05..4ea44dcc6 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t 
last_update, struct strbuf *que
argv[3] = NULL;
cp.argv = argv;
cp.use_shell = 1;
+   cp.dir = get_git_work_tree();
 
return capture_command(, query_result, 1024);
 }
-- 
2.15.0.rc1.413.g76aedb451

[PATCH v3] 0/4 fsmonitor fixes

Updates since v2:

 - Fix tab which crept into 1/4

 - Fixed the benchmarking code in the commit message in 2/4 to just
   always load JSON::XS -- the previous version was the version where
   I'd broken that to force loading of JSON::PP.

 - Remove the --no-pretty from the t/ version of query-watchman in
   2/4; I don't know how I messed up diff'ing the file previously, but
   if there are already differences, it makes sense to let them slide.

[PATCH v3 3/4] fsmonitor: Document GIT_TRACE_FSMONITOR

Signed-off-by: Alex Vandiver 
---
 Documentation/git.txt | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 1fca63634..720db196e 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -594,6 +594,10 @@ into it.
 Unsetting the variable, or setting it to empty, "0" or
 "false" (case insensitive) disables trace messages.
 
+`GIT_TRACE_FSMONITOR`::
+   Enables trace messages for the filesystem monitor extension.
+   See `GIT_TRACE` for available trace output options.
+
 `GIT_TRACE_PACK_ACCESS`::
Enables trace messages for all accesses to any packs. For each
access, the pack file name and an offset in the pack is
-- 
2.15.0.rc1.413.g76aedb451

[PATCH v3 4/4] fsmonitor: Delay updating state until after split index is merged

If the fsmonitor extension is used in conjunction with the split index
extension, the set of entries in the index when it is first loaded is
only a subset of the real index.  This leads to only the non-"base"
index being marked as CE_FSMONITOR_VALID.

Delay the expansion of the ewah bitmap until after tweak_split_index
has been called to merge in the base index as well.

The new fsmonitor_dirty is kept from being leaked by dint of being
cleaned up in post_read_index_from, which is guaranteed to be called
after do_read_index in read_index_from.

Signed-off-by: Alex Vandiver 
---
 cache.h |  1 +
 fsmonitor.c | 39 ---
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/cache.h b/cache.h
index 25adcf681..0a4f43ec2 100644
--- a/cache.h
+++ b/cache.h
@@ -348,6 +348,7 @@ struct index_state {
unsigned char sha1[20];
struct untracked_cache *untracked;
uint64_t fsmonitor_last_update;
+   struct ewah_bitmap *fsmonitor_dirty;
 };
 
 extern struct index_state the_index;
diff --git a/fsmonitor.c b/fsmonitor.c
index 4ea44dcc6..417759224 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -49,20 +49,7 @@ int read_fsmonitor_extension(struct index_state *istate, 
const void *data,
ewah_free(fsmonitor_dirty);
return error("failed to parse ewah bitmap reading fsmonitor 
index extension");
}
-
-   if (git_config_get_fsmonitor()) {
-   /* Mark all entries valid */
-   for (i = 0; i < istate->cache_nr; i++)
-   istate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;
-
-   /* Mark all previously saved entries as dirty */
-   ewah_each_bit(fsmonitor_dirty, fsmonitor_ewah_callback, istate);
-
-   /* Now mark the untracked cache for fsmonitor usage */
-   if (istate->untracked)
-   istate->untracked->use_fsmonitor = 1;
-   }
-   ewah_free(fsmonitor_dirty);
+   istate->fsmonitor_dirty = fsmonitor_dirty;
 
trace_printf_key(_fsmonitor, "read fsmonitor extension 
successful");
return 0;
@@ -239,7 +226,29 @@ void remove_fsmonitor(struct index_state *istate)
 
 void tweak_fsmonitor(struct index_state *istate)
 {
-   switch (git_config_get_fsmonitor()) {
+   int i;
+   int fsmonitor_enabled = git_config_get_fsmonitor();
+
+   if (istate->fsmonitor_dirty) {
+   if (fsmonitor_enabled) {
+   /* Mark all entries valid */
+   for (i = 0; i < istate->cache_nr; i++) {
+   istate->cache[i]->ce_flags |= 
CE_FSMONITOR_VALID;
+   }
+
+   /* Mark all previously saved entries as dirty */
+   ewah_each_bit(istate->fsmonitor_dirty, 
fsmonitor_ewah_callback, istate);
+
+   /* Now mark the untracked cache for fsmonitor usage */
+   if (istate->untracked)
+   istate->untracked->use_fsmonitor = 1;
+   }
+
+   ewah_free(istate->fsmonitor_dirty);
+   istate->fsmonitor_dirty = NULL;
+   }
+
+   switch (fsmonitor_enabled) {
case -1: /* keep: do nothing */
break;
case 0: /* false */
-- 
2.15.0.rc1.413.g76aedb451

[PATCH v3 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman

This provides modest performance savings.  Benchmarking with the
following program, with and without `--no-pretty`, we find savings of
23% (0.316s -> 0.242s) in the git repository, and savings of 8% (5.24s
-> 4.86s) on a large repository with 580k files in the working copy.

#!/usr/bin/perl

use strict;
use warnings;
use IPC::Open2;
use JSON::XS;

my $pid = open2(\*CHLD_OUT, \*CHLD_IN, "watchman -j @ARGV")
or die "open2() failed: $!\n" .
"Falling back to scanning...\n";

my $query = qq|["query", "$ENV{PWD}", {}]|;

print CHLD_IN $query;
close CHLD_IN;
my $response = do {local $/; };

JSON::XS->new->utf8->decode($response);

Signed-off-by: Alex Vandiver 
---
 templates/hooks--fsmonitor-watchman.sample | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/templates/hooks--fsmonitor-watchman.sample 
b/templates/hooks--fsmonitor-watchman.sample
index 9eba8a740..9a082f278 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -49,7 +49,7 @@ launch_watchman();
 
 sub launch_watchman {
 
-   my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j')
+   my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')
or die "open2() failed: $!\n" .
"Falling back to scanning...\n";
 
-- 
2.15.0.rc1.413.g76aedb451

Re: grep vs git grep performance?

2017-10-27 Thread Joe Perches

On Sat, 2017-10-28 at 00:11 +0200, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Oct 27 2017, Joe Perches jotted:
[]
> > git grep performance has already been
> > quite successfully improved.
> 
> ...and I have WIP patches to use the PCRE engine for patterns without -P
> which I intend to start sending soon after the next release.

One addition that would be quite nice would be
an option to have regex matches span input lines.

grep v2.54 was the last grep version that allowed
this and I keep it around just for that.

ie:

$ cat hello.txt 
Hello
World
$ grep -P "Hello\s*World" hello.txt 
$ grep-2.5.4 -P "Hello\s*World" hello.txt 
Hello
World

Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree

On Thu, 26 Oct 2017, Ben Peart wrote:
> I saw your response but actually can't replicate it locally.  I've been
> running with Watchman integration on all my repos for months and my "watchman
> watch-list" command only shows the root of the various working directories -
> no subdirectories.

Weird.  I double-checked and I see the same behavior with watchman
4.9.0 as with 4.7.0 that I had been using previously.  I wonder if
something's different between `git` in `next` from wherever your
branch was based.
 - Alex

[PATCH 0/3] convert diff flags to be stored in a struct

There has be some desire to add additional flags to the diff machineery
(https://public-inbox.org/git/20171024000931.14814-1-sbel...@google.com/) but
due to the limits of the number of bits in an unsigned int on some systems we
can't add any additonal flags to the 'flags' variable.  This series converts
the flags to be stored in bitfields in a struct instead of in bit positions in
an unsigned int.

Some thoughts:
 * We may want to do a follow on patch to convert all the flags from being in
   uppercase to lower case.
 * Maybe we can figure out how to remove the 'touched_flags' things (since its
   only used in one place) and then we may even be able to stop needing to use
   macros to set/clr/test the flags.

Brandon Williams (3):
  add: use DIFF_OPT_SET macro to set a diff flag
  reset: use DIFF_OPT_SET macro to set a diff flag
  diff: convert flags to be stored in bitfields

 builtin/add.c|  2 +-
 builtin/commit.c |  7 +++--
 builtin/log.c|  2 +-
 builtin/reset.c  |  2 +-
 diff-lib.c   |  6 ++--
 diff.c   |  3 +-
 diff.h   | 96 +---
 sequencer.c  |  5 +--
 8 files changed, 72 insertions(+), 51 deletions(-)

-- 
2.15.0.rc2.357.g7e34df9404-goog

[PATCH 2/3] reset: use DIFF_OPT_SET macro to set a diff flag

Instead of explicitly setting the 'DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG'
flag, use the 'DIFF_OPT_SET' macro.

Signed-off-by: Brandon Williams 
---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 9cd89b230..ea2fad5a0 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -166,7 +166,7 @@ static int read_from_tree(const struct pathspec *pathspec,
opt.output_format = DIFF_FORMAT_CALLBACK;
opt.format_callback = update_index_from_diff;
opt.format_callback_data = _to_add;
-   opt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
+   DIFF_OPT_SET(, OVERRIDE_SUBMODULE_CONFIG);
 
if (do_diff_cache(tree_oid, ))
return 1;
-- 
2.15.0.rc2.357.g7e34df9404-goog

[PATCH 1/3] add: use DIFF_OPT_SET macro to set a diff flag

Instead of explicitly setting the 'DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG'
flag, use the 'DIFF_OPT_SET' macro.

Signed-off-by: Brandon Williams 
---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index a648cf4c5..b70e8a779 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -116,7 +116,7 @@ int add_files_to_cache(const char *prefix,
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = update_callback;
rev.diffopt.format_callback_data = 
-   rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
+   DIFF_OPT_SET(, OVERRIDE_SUBMODULE_CONFIG);
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
run_diff_files(, DIFF_RACY_IS_MODIFIED);
clear_pathspec(_data);
-- 
2.15.0.rc2.357.g7e34df9404-goog

[PATCH 3/3] diff: convert flags to be stored in bitfields

We have have reached the limit of the number of flags that can be stored
in a single unsigned int.  In order to allow for more flags to be added
to the diff machinery in the future this patch converts the flags to be
stored in bitfields in 'struct diff_flags'.

Signed-off-by: Brandon Williams 
---
 builtin/commit.c |  7 +++--
 builtin/log.c|  2 +-
 diff-lib.c   |  6 ++--
 diff.c   |  3 +-
 diff.h   | 96 +---
 sequencer.c  |  5 +--
 6 files changed, 70 insertions(+), 49 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d75b3805e..de08c2594 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -912,11 +912,12 @@ static int prepare_to_commit(const char *index_file, 
const char *prefix,
 * submodules which were manually staged, which would
 * be really confusing.
 */
-   int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
+   struct diff_flags flags = DIFF_FLAGS_INIT;
+   flags.OVERRIDE_SUBMODULE_CONFIG = 1;
if (ignore_submodule_arg &&
!strcmp(ignore_submodule_arg, "all"))
-   diff_flags |= DIFF_OPT_IGNORE_SUBMODULES;
-   commitable = index_differs_from(parent, diff_flags, 1);
+   flags.IGNORE_SUBMODULES = 1;
+   commitable = index_differs_from(parent, flags, 1);
}
}
strbuf_release(_ident);
diff --git a/builtin/log.c b/builtin/log.c
index d81a09051..780975ed4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -134,7 +134,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 
if (default_date_mode)
parse_date_format(default_date_mode, >date_mode);
-   rev->diffopt.touched_flags = 0;
+   rev->diffopt.touched_flags = diff_flags_cleared;
 }
 
 static void cmd_log_init_finish(int argc, const char **argv, const char 
*prefix,
diff --git a/diff-lib.c b/diff-lib.c
index 4e0980caa..7375ef71d 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -71,7 +71,7 @@ static int match_stat_with_submodule(struct diff_options 
*diffopt,
 {
int changed = ce_match_stat(ce, st, ce_option);
if (S_ISGITLINK(ce->ce_mode)) {
-   unsigned orig_flags = diffopt->flags;
+   struct diff_flags orig_flags = diffopt->flags;
if (!DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG))
set_diffopt_flags_from_submodule_config(diffopt, 
ce->name);
if (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))
@@ -534,7 +534,7 @@ int do_diff_cache(const struct object_id *tree_oid, struct 
diff_options *opt)
return 0;
 }
 
-int index_differs_from(const char *def, int diff_flags,
+int index_differs_from(const char *def, struct diff_flags flags,
   int ita_invisible_in_index)
 {
struct rev_info rev;
@@ -546,7 +546,7 @@ int index_differs_from(const char *def, int diff_flags,
setup_revisions(0, NULL, , );
DIFF_OPT_SET(, QUICK);
DIFF_OPT_SET(, EXIT_WITH_STATUS);
-   rev.diffopt.flags |= diff_flags;
+   rev.diffopt.flags = diff_flags_or(rev.diffopt.flags, flags);
rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;
run_diff_index(, 1);
object_array_clear();
diff --git a/diff.c b/diff.c
index 6fd288420..6f1050d42 100644
--- a/diff.c
+++ b/diff.c
@@ -46,6 +46,7 @@ static int diff_no_prefix;
 static int diff_stat_graph_width;
 static int diff_dirstat_permille_default = 30;
 static struct diff_options default_diff_options;
+const struct diff_flags diff_flags_cleared = DIFF_FLAGS_INIT;
 static long diff_algorithm;
 static unsigned ws_error_highlight_default = WSEH_NEW;
 
@@ -5899,7 +5900,7 @@ int diff_can_quit_early(struct diff_options *opt)
 static int is_submodule_ignored(const char *path, struct diff_options *options)
 {
int ignored = 0;
-   unsigned orig_flags = options->flags;
+   struct diff_flags orig_flags = options->flags;
if (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))
set_diffopt_flags_from_submodule_config(options, path);
if (DIFF_OPT_TST(options, IGNORE_SUBMODULES))
diff --git a/diff.h b/diff.h
index aca150ba2..d58f06106 100644
--- a/diff.h
+++ b/diff.h
@@ -60,42 +60,59 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct 
diff_options *opt, void *data)
 
 #define DIFF_FORMAT_CALLBACK   0x1000
 
-#define DIFF_OPT_RECURSIVE   (1 <<  0)
-#define DIFF_OPT_TREE_IN_RECURSIVE   (1 <<  1)
-#define DIFF_OPT_BINARY  (1 <<  2)
-#define DIFF_OPT_TEXT(1 <<  3)
-#define DIFF_OPT_FULL_INDEX  (1 <<  4)
-#define DIFF_OPT_SILENT_ON_REMOVE(1 <<  5)
-#define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
-#define DIFF_OPT_FOLLOW_RENAMES  (1 <<

Re: grep vs git grep performance?

2017-10-27 Thread Ævar Arnfjörð Bjarmason


On Fri, Oct 27 2017, Joe Perches jotted:

> On Thu, 2017-10-26 at 10:45 -0700, Stefan Beller wrote:
>> On Thu, Oct 26, 2017 at 10:41 AM, Joe Perches  wrote:
>> > On Thu, 2017-10-26 at 09:58 -0700, Stefan Beller wrote:
>> > > + Avar who knows a thing about pcre (I assume the regex compilation
>> > > has impact on grep speed)
>> > >
>> > > On Thu, Oct 26, 2017 at 8:02 AM, Joe Perches  wrote:
>> > > > Comparing a cache warm git grep vs command line grep
>> > > > shows significant differences in cpu & wall clock.
>> > > >
>> > > > Any ideas how to improve this?
>> > > >
>> > > > $ time git grep "\bseq_.*%p\W" | wc -l
>> > > > 112
>> > > >
>> > > > real0m4.271s
>> > > > user0m15.520s
>> > > > sys 0m0.395s
>> > > >
>> > > > $ time grep -r --include=*.[ch] "\bseq_.*%p\W" * | wc -l
>> > > > 112
>> > > >
>> > > > real0m1.164s
>> > > > user0m0.847s
>> > > > sys 0m0.314s
>> > > >
>> > >
>> > > I wonder how much is algorithmic advantage vs coding/micro
>> > > optimization that we can do.
>> >
>> > As do I.  I presume this is libpcre related.
>> >
>> > For instance, git grep performance is better than grep for:
>> >
>> > $ time git grep -w "seq_printf" -- "*.[ch]" | wc -l
>> > 8609
>> >
>> > real0m0.301s
>> > user0m0.548s
>> > sys 0m0.372s
>> >
>> > $ time grep -w -r --include=*.[ch] "seq_printf" * | wc -l
>> > 8609
>> >
>> > real0m0.706s
>> > user0m0.396s
>> > sys 0m0.309s
>> >
>>
>> One important piece of information is what version of Git you are running,
>>
>>
>> $ git tag --contains origin/ab/pcre-v2
>> v2.14.0
>
> v2.10
>
>> ...
>>
>> (and the version of pcre, see the numbers)
>> https://git.kernel.org/pub/scm/git/git.git/commit/?id=94da9193a6eb8f1085d611c04ff8bbb4f5ae1e0a
>
> I definitely didn't have that one.
>
> I recompiled git latest (with USE_LIBPCRE2) and reran.
>
> Here are the results
>
> $ git --version
> git version 2.15.0.rc2.48.g4e40fb3
>
> $ time git grep -P "\bseq_.*%p\W" -- "*.[ch]" | wc -l
> 112
>
> real  0m0.437s
> user  0m1.008s
> sys   0m0.381s
>
> So, git grep performance has already been
> quite successfully improved.

...and I have WIP patches to use the PCRE engine for patterns without -P
which I intend to start sending soon after the next release.

Re: [PATCH v16 Part II 6/8] bisect--helper: `get_terms` & `bisect_terms` shell function in C

2017-10-27 Thread Martin Ågren

On 27 October 2017 at 17:06, Pranit Bauva  wrote:
> +   for (i = 0; i < argc; i++) {
> +   if (!strcmp(argv[i], "--term-good"))
> +   printf(_("%s\n"), terms->term_good);
> +   else if (!strcmp(argv[i], "--term-bad"))
> +   printf(_("%s\n"), terms->term_bad);

You seem to have lost --term-old and --term-new. I also wonder why these
would need translating. You break GETTEXT_POISON here, then fix it in
patch 8/8.

I'm not even sure you need patch 8/8. If I drop these two `_()`, I can
run `git rebase -ix "make GETTEXT_POISON=Yes test"` on the entire series
without failure. Patch 8/8 also switches to `test_i18ngrep` for some
usages of `git branch` and for some checks on `.git/BISECT_START`. I'm
not sure that's needed.

> +   else
> +   error(_("BUG: invalid argument %s for 'git bisect 
> terms'.\n"
> + "Supported options are: "
> + "--term-good|--term-old and "
> + "--term-bad|--term-new."), argv[i]);
> +   }

Re: What's cooking in git.git (Oct 2017, #06; Fri, 27)

> * sb/submodule-recursive-checkout-detach-head (2017-07-28) 2 commits
>   (merged to 'next' on 2017-10-26 at 30994b4c76)
>  + Documentation/checkout: clarify submodule HEADs to be detached
>  + recursive submodules: detach HEAD from new state
>
>  "git checkout --recursive" may overwrite and rewind the history of
>  the branch that happens to be checked out in submodule
>  repositories, which might not be desirable.  Detach the HEAD but
>  still allow the recursive checkout to succeed in such a case.
>
>  Undecided.
>  This needs justification in a larger picture; it is unclear why
>  this is better than rejecting recursive checkout, for example.

We have been running this patch internally for a couple of weeks now,
and had no complaints by users.

Detaching the submodule HEAD is in line with the current thinking
of submodules, though I am about to send out a plan later
asking if we want to keep it that way long term.

Thanks,
Stefan

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

Pranit Bauva  writes:

> - bisect_write "$state" "$rev"
> + git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" 
> "$TERM_BAD" || exit

I can see why two extra "terms" parameters need to be passed to this
helper at this step; looking at patches around 4/8 and 6/8 where C
code can directly find out what words are used for GOOD and BAD, we
should be able to lose these two extra parameters from this helper
by internally making a call to get_terms() from bisect_write() ;-)

Re: [PATCH v16 Part II 1/8] bisect--helper: `bisect_reset` shell function in C

Pranit Bauva  writes:

> +static int bisect_reset(const char *commit)
> +{
> + struct strbuf branch = STRBUF_INIT;
> +
> + if (!commit) {
> + if (strbuf_read_file(, git_path_bisect_start(), 0) < 1)
> + return !printf(_("We are not bisecting.\n"));
> + strbuf_rtrim();
> + } else {
> + struct object_id oid;
> +
> + if (get_oid_commit(commit, ))
> + return error(_("'%s' is not a valid commit"), commit);
> + strbuf_addstr(, commit);

The original checks "test -s BISECT_START" and complains, even when
an explicit commit is given.  With this change, when the user is not
bisecting, giving "git bisect reset master" goes ahead---it is
likely that BISECT_HEAD does not exist and we may hit "Could not
check out" error, but if BISECT_HEAD is left behind from a previous
run (which is likely completely unrelated to whatever the user
currently is doing), we'd end up doing quite a random thing, no?

> + }
> +
> + if (!file_exists(git_path_bisect_head())) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
> +
> + argv_array_pushl(, "checkout", branch.buf, "--", NULL);
> + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
> + error(_("Could not check out original HEAD '%s'. Try "
> + "'git bisect reset '."), branch.buf);
> + strbuf_release();
> + argv_array_clear();
> + return -1;

How does this return value affect the value eventually given to
exit(3), called by somewhere in git.c that called this function?

The call graph would be

common-main.c::main()
-> git.c::cmd_main()
   -> handle_builtin()
  . exit(run_builtin())
  -> run_builtin()
 . status = p->fn()
 -> cmd_bisect__helper()
. return bisect_reset()
-> bisect_reset()
   . return -1
 . if (status) return status;

So the -1 is returned throughout the callchain and exit(3) ends up
getting it---which is not quite right.  We shouldn't be giving
negative value to exit(3).  bisect_clean_state() and other helper
functions may already share the same issue.

> + }
> + argv_array_clear();
> + }
> +
> + strbuf_release();
> + return bisect_clean_state();
> +}

Re: [PATCH v16 Part II 5/8] bisect--helper: `bisect_next_check` shell function in C

2017-10-27 Thread Martin Ågren

On 27 October 2017 at 17:06, Pranit Bauva  wrote:
> +   /*
> +* have bad (or new) but not good (or old). We could bisect
> +* although this is less optimum.
> +*/
> +   fprintf(stderr, _("Warning: bisecting only with a %s 
> commit\n"),
> +   terms->term_bad);

Maybe this should use `warning()`?

> -   # have bad (or new) but not good (or old).  we could bisect 
> although
> -   # this is less optimum.
> -   eval_gettextln "Warning: bisecting only with a \$TERM_BAD 
> commit." >&2

I wonder if we can somehow pick up the existing translation? It would
now be fuzzy, in some sense, but since the string was originally in a
different file, maybe the po-tools won't be able to discover the
fuzzyness? We could add a TRANSLATORS-comment, so that the translators
know that this string matches an old one. There are more strings like
that in this patch, and maybe in some others as well, I haven't looked.

(Adding Jiang to cc.)

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

2017-10-27 Thread Martin Ågren

On 27 October 2017 at 17:06, Pranit Bauva  wrote:
> +static void free_terms(struct bisect_terms *terms)
> +{
> +   if (!terms->term_good)
> +   free((void *) terms->term_good);
> +   if (!terms->term_bad)
> +   free((void *) terms->term_bad);
> +}

These look like no-ops. Remove `!` for correctness, or `if (...)` for
simplicity, since `free()` can handle NULL.

You leave the pointers dangling, but you're ok for now since this is the
last thing that happens in `cmd_bisect__helper()`. Your later patches
add more users, but they're also ok, since they immediately assign new
values.

In case you (and others) find it useful, the below is a patch I've been
sitting on for a while as part of a series to plug various memory-leaks.
`FREE_AND_NULL_CONST()` would be useful in precisely these situations.

-- >8 --
Subject: git-compat-util: add FREE_AND_NULL_CONST() wrapper

Commit 481df65 ("git-compat-util: add a FREE_AND_NULL() wrapper around
free(ptr); ptr = NULL", 2017-06-15) added `FREE_AND_NULL()`. One
advantage of this macro is that it reduces risk of copy-paste errors and
reviewer-fatigue, especially when cleaning up more than just a single
pointer.

However, `FREE_AND_NULL()` can not be used with a const-pointer:

  struct foo { const char *bar; }
  ...
  FREE_AND_NULL(baz.bar);

This causes the compiler to barf as `free()` is called: "error: passing
argument 1 of ‘free’ discards ‘const’ qualifier from pointer target
type". A naive attempt to remedy this might look like this:

  FREE_AND_NULL((void *)baz.bar);

Now the assignment is problematic: "error: lvalue required as left
operand of assignment".

Add a `FREE_AND_NULL_CONST()` wrapper macro which acts as
`FREE_AND_NULL()`, except it casts to `(void *)` when freeing. As a
demonstration, use this in two existing code paths that were identified
by some grepping. Future patches will use it to clean up struct-fields:

  FREE_AND_NULL_CONST(baz.bar);

This macro is a slightly bigger hammer than `FREE_AND_NULL` and has a
slightly larger potential for misuse. Document that `FREE_AND_NULL`
should be preferred.

Signed-off-by: Martin Ågren 
---
 argv-array.c  | 3 +--
 git-compat-util.h | 8 
 submodule.c   | 3 +--
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/argv-array.c b/argv-array.c
index 5d370fa33..433a5997a 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -59,8 +59,7 @@ void argv_array_pop(struct argv_array *array)
 {
if (!array->argc)
return;
-   free((char *)array->argv[array->argc - 1]);
-   array->argv[array->argc - 1] = NULL;
+   FREE_AND_NULL_CONST(array->argv[array->argc - 1]);
array->argc--;
 }

diff --git a/git-compat-util.h b/git-compat-util.h
index cedad4d58..ca3dcbc58 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -815,6 +815,14 @@ extern FILE *fopen_or_warn(const char *path, const char 
*mode);
  */
 #define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)

+/*
+ * FREE_AND_NULL_CONST(ptr) is like FREE_AND_NULL(ptr) except it casts
+ * to (void *) when calling free. This is useful when handling, e.g., a
+ * `const char *`, but it can also be misused. Prefer FREE_AND_NULL, and
+ * use this only when you need to and it is safe to do so.
+ */
+#define FREE_AND_NULL_CONST(p) do { free((void *)(p)); (p) = NULL; } while (0)
+
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), 
(alloc)))

diff --git a/submodule.c b/submodule.c
index 63e7094e1..7759f9050 100644
--- a/submodule.c
+++ b/submodule.c
@@ -364,8 +364,7 @@ int parse_submodule_update_strategy(const char *value,
 {
enum submodule_update_type type;

-   free((void*)dst->command);
-   dst->command = NULL;
+   FREE_AND_NULL_CONST(dst->command);

type = parse_submodule_update_type(value);
if (type == SM_UPDATE_UNSPECIFIED)
-- 
2.15.0.rc2.5.g97dd00bb7

Re: [PATCH v3] merge-recursive: check GIT_MERGE_VERBOSITY only once

On Wed, Oct 25, 2017 at 6:03 AM, Andrey Okoshkin  wrote:
> Get 'GIT_MERGE_VERBOSITY' environment variable only once in
> init_merge_options() and store the pointer to its value for the further check.
> No intervening calls to getenv(), putenv(), setenv() or unsetenv() are done
> between the initial getenv() call and the consequential result pass to 
> strtol()
> as these environment related functions could modify the string pointer 
> returned
> by the initial getenv() call.
>
> Signed-off-by: Andrey Okoshkin 

This is
Reviewed-by: Stefan Beller

Re: grep vs git grep performance?

2017-10-27 Thread Joe Perches

On Thu, 2017-10-26 at 10:45 -0700, Stefan Beller wrote:
> On Thu, Oct 26, 2017 at 10:41 AM, Joe Perches  wrote:
> > On Thu, 2017-10-26 at 09:58 -0700, Stefan Beller wrote:
> > > + Avar who knows a thing about pcre (I assume the regex compilation
> > > has impact on grep speed)
> > > 
> > > On Thu, Oct 26, 2017 at 8:02 AM, Joe Perches  wrote:
> > > > Comparing a cache warm git grep vs command line grep
> > > > shows significant differences in cpu & wall clock.
> > > > 
> > > > Any ideas how to improve this?
> > > > 
> > > > $ time git grep "\bseq_.*%p\W" | wc -l
> > > > 112
> > > > 
> > > > real0m4.271s
> > > > user0m15.520s
> > > > sys 0m0.395s
> > > > 
> > > > $ time grep -r --include=*.[ch] "\bseq_.*%p\W" * | wc -l
> > > > 112
> > > > 
> > > > real0m1.164s
> > > > user0m0.847s
> > > > sys 0m0.314s
> > > > 
> > > 
> > > I wonder how much is algorithmic advantage vs coding/micro
> > > optimization that we can do.
> > 
> > As do I.  I presume this is libpcre related.
> > 
> > For instance, git grep performance is better than grep for:
> > 
> > $ time git grep -w "seq_printf" -- "*.[ch]" | wc -l
> > 8609
> > 
> > real0m0.301s
> > user0m0.548s
> > sys 0m0.372s
> > 
> > $ time grep -w -r --include=*.[ch] "seq_printf" * | wc -l
> > 8609
> > 
> > real0m0.706s
> > user0m0.396s
> > sys 0m0.309s
> > 
> 
> One important piece of information is what version of Git you are running,
> 
> 
> $ git tag --contains origin/ab/pcre-v2
> v2.14.0

v2.10

> ...
> 
> (and the version of pcre, see the numbers)
> https://git.kernel.org/pub/scm/git/git.git/commit/?id=94da9193a6eb8f1085d611c04ff8bbb4f5ae1e0a

I definitely didn't have that one.

I recompiled git latest (with USE_LIBPCRE2) and reran.

Here are the results

$ git --version
git version 2.15.0.rc2.48.g4e40fb3

$ time git grep -P "\bseq_.*%p\W" -- "*.[ch]" | wc -l
112

real0m0.437s
user0m1.008s
sys 0m0.381s

So, git grep performance has already been
quite successfully improved.

Thanks.

Re: [PATCH 1/2] xdiff-interface: export comparing and hashing strings

On Fri, Oct 27, 2017 at 12:12 AM, Junio C Hamano  wrote:
> René Scharfe  writes:
>
>> Am 25.10.2017 um 20:49 schrieb Stefan Beller:
>>> +/*
>>> + * Compare the strings l1 with l2 which are of size s1 and s2 respectively.
>>> + * Returns 1 if the strings are deemed equal, 0 otherwise.
>>> + * The `flags` given as XDF_WHITESPACE_FLAGS determine how white spaces
>>> + * are treated for the comparision.
>>> + */
>>> +extern int xdiff_compare_lines(const char *l1, long s1,
>>> +   const char *l2, long s2, long flags);
>>
>> With the added comment it's OK here.
>>
>> Still, I find the tendency in libxdiff to use the shortest possible
>> variable names to be hard on the eyes.  That math-like notation may have
>> its place in that external library, but I think we should be careful
>> lest it spreads.
>
> Yeah, I tend to agree.  The xdiff-interface is at the (surprise!)
> interface layer, so we could make it follow the style of either one,
> but we seem to have picked up the convention of the lower layer,
> so...
>
> By the way, I was looking at the code around this area while
> reviewing the cr-at-eol thing, and noticed a couple of things:
>
>
>  * combine-diff.c special cases only IGNORE_WHITESPACE and
>IGNORE_WHITESPACE_CHANGE but no other IGNORE_WHITESPACE things; I
>have a suspicion that this is not because IGNORE_WHITESPACE_AT_EOL
>does not have to special cased by the codepath, but only because
>the combine-diff.c refactoring predates the introduction of
>ws-at-eol thing?

I wonder how much overlap between the IGNORE_WHITESPACE_AT_EOL
and CRLF-AT-EOL is (maybe these can be combined, as per the root of
this discussion)

>The machinery uses its own match_string_spaces() helper; it may
>make sense to use the same xdiff_compare_lines() in its callers
>and get rid of this helper function.

Speaking of xdiff_compare_lines, it serves the special purpose of the
move detection as well as its internal use cases, but we may need to
change its interface/implementation in the future, to align it with strcmp,
currently the compare function only returns equality, not an order.

>  * diff_setup_done() sets DIFF_FROM_CONTENTS when any of the
>IGNORE_WHITESPACE bits is true, to allow "git diff -q
>--ignore-something" would do the right thing.  We do not however
>give a similar special case to XDF_IGNORE_BLANK_LINES.
>
>$ echo >>Makefile && git diff $option --ignore-blank-lines Makefile
>
>exits with 1 when option=--exit-code and it exits with 0 when
>option=-q
>
>
> For now I'll leve these as #leftoverbits, but I may come back to the
> latter soonish.  I won't come back to the former until Stefan's
> refactor hits 'master', though.

which is presumably after the 2.15 release.

To tack on the #leftoverbits: The --color-moved detection doesn't
pay attention to XDF_IGNORE_BLANK_LINES (which is tricky as
it is on the per-line layer. If we want to ignore spurious blank lines,
we'd have to check for this flag in diff.c in mark_color_as_moved(..)
in the block
/* Check any potential block runs, advance each or nullify */

Thanks,
Stefan

[PATCH 2/2] xdiff/xdiffi.c: remove unneeded function declarations

There is no need to forward-declare these functions, as they are used
after their implementation only.

Signed-off-by: Stefan Beller 
---
 xdiff/xdiffi.c | 17 -
 1 file changed, 17 deletions(-)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 93a65680a1..0a4ea948f9 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -22,34 +22,17 @@
 
 #include "xinclude.h"
 
-
-
 #define XDL_MAX_COST_MIN 256
 #define XDL_HEUR_MIN_COST 256
 #define XDL_LINE_MAX (long)((1UL << (CHAR_BIT * sizeof(long) - 1)) - 1)
 #define XDL_SNAKE_CNT 20
 #define XDL_K_HEUR 4
 
-
-
 typedef struct s_xdpsplit {
long i1, i2;
int min_lo, min_hi;
 } xdpsplit_t;
 
-
-
-
-static long xdl_split(unsigned long const *ha1, long off1, long lim1,
- unsigned long const *ha2, long off2, long lim2,
- long *kvdf, long *kvdb, int need_min, xdpsplit_t *spl,
- xdalgoenv_t *xenv);
-static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long 
chg1, long chg2);
-
-
-
-
-
 /*
  * See "An O(ND) Difference Algorithm and its Variations", by Eugene Myers.
  * Basically considers a "box" (off1, off2, lim1, lim2) and scan from both
-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCH 0/2] Re* Consequences of CRLF in index?

> Let's do this bit-shuffling as a preliminary clean-up.

These 2 patches can go on top of that as well.

Thanks,
Stefan

Stefan Beller (2):
  xdiff/xdiff.h: remove unused flags
  xdiff/xdiffi.c: remove unneeded function declarations

 xdiff/xdiff.h  |  8 
 xdiff/xdiffi.c | 17 -
 2 files changed, 25 deletions(-)

-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCH 1/2] xdiff/xdiff.h: remove unused flags

These flags were there since the beginning (3443546f6e (Use a *real*
built-in diff generator, 2006-03-24), but were never used. Remove them.

Signed-off-by: Stefan Beller 
---
 xdiff/xdiff.h | 8 
 1 file changed, 8 deletions(-)

diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 457cac32d8..0a41f5e500 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -48,14 +48,6 @@ extern "C" {
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)
 
-#define XDL_MMB_READONLY (1 << 0)
-
-#define XDL_MMF_ATOMIC (1 << 0)
-
-#define XDL_BDOP_INS 1
-#define XDL_BDOP_CPY 2
-#define XDL_BDOP_INSB 3
-
 /* merge simplification levels */
 #define XDL_MERGE_MINIMAL 0
 #define XDL_MERGE_EAGER 1
-- 
2.15.0.rc2.443.gfcc3b81c0a

Re: Re* Consequences of CRLF in index?

On Thu, Oct 26, 2017 at 11:13 PM, Junio C Hamano  wrote:
> Subject: [PATCH] xdiff: reassign xpparm_t.flags bits
>
> We have packed the bits too tightly in such a way that it is not
> easy to add a new type of whitespace ignoring option, a new type
> of LCS algorithm, or a new type of post-cleanup heuristics.
>
> Reorder bits a bit to give room for these three classes of options
> to grow.
>
> While at it, add a comment in front of the bit definitions to
> clarify in which structure these defined bits may appear.
>
> Signed-off-by: Junio C Hamano 

Reviewed-by: Stefan Beller 

> ---
>  xdiff/xdiff.h | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
> index b090ad8eac..457cac32d8 100644
> --- a/xdiff/xdiff.h
> +++ b/xdiff/xdiff.h
> @@ -27,22 +27,24 @@
>  extern "C" {
>  #endif /* #ifdef __cplusplus */
>
> +/* xpparm_t.flags */
> +#define XDF_NEED_MINIMAL (1 << 0)
>
> -#define XDF_NEED_MINIMAL (1 << 1)

The whitespace flags are also stored in xpparm_t, though
these flags can also come in from other sources (e.g. we
just pass it in manually via the interface).

>  #define XDF_IGNORE_WHITESPACE (1 << 2)
>  #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
>  #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)
>  #define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | 
> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)
>
> -#define XDF_PATIENCE_DIFF (1 << 5)
> -#define XDF_HISTOGRAM_DIFF (1 << 6)
> +#define XDF_IGNORE_BLANK_LINES (1 << 7)

Is XDF_IGNORE_BLANK_LINES a whitespace option, just
on the lines level instead of the inside one line? I think
it still makes sense to keep it here, slightly separated from
the IGNORE flags.

> +#define XDF_PATIENCE_DIFF (1 << 14)
> +#define XDF_HISTOGRAM_DIFF (1 << 15)
>  #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)
>  #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)
>
> -#define XDF_IGNORE_BLANK_LINES (1 << 7)
> -
> -#define XDF_INDENT_HEURISTIC (1 << 8)
> +#define XDF_INDENT_HEURISTIC (1 << 23)
>
> +/* xdemitconf_t.flags */
>  #define XDL_EMIT_FUNCNAMES (1 << 0)
>  #define XDL_EMIT_FUNCCONTEXT (1 << 2)

This looked like it was carefully crafted to avoid accidental bit overlap
with XDF_NEED_MINIMAL; but these are in different flag fields, this
should be fine.

Thanks,
Stefan

Re: [PATCH] diff: fix lstat() error handling in diff_populate_filespec()

Junio C Hamano  writes:

> Andrey Okoshkin  writes:
>
>> I'm not sure why only ENOENT error of lstat() is considered as an
>> error but passing by other errno values leads to reading of
>> uninitialized 'struct stat st' variable.  It means that the
>> populated 'diff_filespec' structure may be incorrectly filled.
>
> Entirely correct.  There is no fundamental reason to try special
> casing ENOENT, unless we are clearing the "this is an error" bit
> when the errno is ENOENT---but this code does not even do so.  All
> errors are errors---we wanted to know the result of lstat() to carry
> on, and we couldn't figure out the status.  We do not want to die
> immediately (instead we want to show diffs for other paths), so
> substituting the result with an empty string is the least bad thing
> we can do at this point in the code.

By the way, if the reason why we are hitting this lstat(2) (not the
reason why lstat(2) is failing) is not because !s->oid.valid
(i.e. we are reading the working tree side because that is what is
on one side of the comparison) but because reuse_worktree_file()
told us to (i.e. we actually are trying to fill the filespec with
the blob contents, but found that we have already the same content
in a working tree file and found it more efficient to read from
there, rather than doing the read_sha1_file() on the s->oid.hash),
it probably is better to fall back to the other side of the if/else
when we see an error from this lstat(2) and pretend as if we got
false from reuse_worktree_file().  

That would allow us continue with the correct information we
originally wanted to use; after all, reusing is merely an
optimization.

Re: [PATCH] diff: fix lstat() error handling in diff_populate_filespec()

Andrey Okoshkin  writes:

> I'm not sure why only ENOENT error of lstat() is considered as an
> error but passing by other errno values leads to reading of
> uninitialized 'struct stat st' variable.  It means that the
> populated 'diff_filespec' structure may be incorrectly filled.

Entirely correct.  There is no fundamental reason to try special
casing ENOENT, unless we are clearing the "this is an error" bit
when the errno is ENOENT---but this code does not even do so.  All
errors are errors---we wanted to know the result of lstat() to carry
on, and we couldn't figure out the status.  We do not want to die
immediately (instead we want to show diffs for other paths), so
substituting the result with an empty string is the least bad thing
we can do at this point in the code.

How to re-merge paths differently?

2017-10-27 Thread Sergey Organov

Hello,

Is there anything like this:

$ git merge b
[... lot of conflicts ...]
$ git re-merge -X ours -- x/   # Leaves 0 conflicts in x/
$ git re-merge -X theirs -- y/ # Leaves 0 conflicts in y/
[... resolve the rest of conflicts manually ...]
$ git commit

[*] I do mean '-X' above, not '-s'.

-- Sergey

Re: [Bug Report] [includeIf] does not parse case insensitive -> case sensitive symlink gitdir

2017-10-27 Thread Torsten Bögershausen

On Fri, Oct 27, 2017 at 09:55:58AM -0400, Adrian Kappel wrote:
> Hello all, not sure if the issue I've come across is a known bug or
> addressable, but wanted to report it just in-case.

Thanks for the detailed description - my question is inline

> 
> 
> ** Summary
> --
> Using the [includeIf] configuration statement with a symlink'd gitdir
> will not work if the symlink is on a case insensitive volume and the
> location it references is on a case sensitive volume.
> 
> ** Steps to reproduce
> --
> 1. Create symlink (case insensitive -> case sensitive):
> /Users/usera/dev -> /Volumes/CaseSensitive/dev
> 2. Create two files: .gitignore and .gitignore-work, both stored in
> /Users/usera/
> 
> .gitconfig
> -
> [user]
>   name = First Last
> 
>   [includeIf "gitdir:~/dev/work"]
> path = .gitconfig-work
> 
> .gitconfig-work
> 
> [user]
>   email = em...@address.com
> 
> 3. cd into a subfolder of ~/dev/work that has been git initialized.
> Let's say ~/dev/work/repo
> 4. Run git config --includes user.email
> 5. See that nothing is output from the command
> 6. Update the [includeIf] statement in .gitconfig to be the real
> location i.e. "gitdir:/Volumes/CaseSensitive/dev/work/repo"

Didn't you set it up pointing do the real location ?
That is what is written above:
> 1. Create symlink (case insensitive -> case sensitive):
> /Users/usera/dev -> /Volumes/CaseSensitive/dev

(I suspect that people use something like this:
 /Users/usera/dev -> /Volumes/casesensitive/dev
 And in this case it would be the file system which says
 casesensitive != CaseSensitive and Git can't do much about it)

> 7. Rerun the command from [4]
> 8. See that em...@address.com is output from the command
> 
> ** Other variations that were not tested
> --
> - symlink on case sensitive volume referencing a location on a case
> insensitive volume
> 
> ** Environment Information
> --
> git --version: 2.14.1
> OS: macOS Sierra 10.12.6
> 
> 
> If a fix is not feasible or likely to be implemented, I would
> recommend that we update the git site's documentation to reflect this
> gotcha. After verification of course.
> 
> Best,
> Adrian Kappel
> akappel

Re: [PATCH v16 Part II 1/8] bisect--helper: `bisect_reset` shell function in C

Hey,

I forgot to mention. One can find the travis build here[1] which is passing.

[1]: https://travis-ci.org/git/git/builds/293725346

Regards,
Pranit Bauva

Re: Consequences of CRLF in index?

Hi Lars,

On Thu, 26 Oct 2017, Lars Schneider wrote:

> > On 26 Oct 2017, at 09:09, Johannes Sixt  wrote:
> > 
> > Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:
> >> I envy you for the blessing of such a clean C++ source that you do not
> >> have any, say, Unix shell script in it. Try this, and weep:
> >>$ printf 'echo \\\r\n\t123\r\n' >a1
> >>$ sh a1
> >>a1: 2: a1: 123: not found
> > 
> > I was bitten by that, too. For this reason, I ensure that shell
> > scripts and Makefiles begin their life on Linux. Fortunately, modern
> > editors on Windows, includ^Wand vi, do not force CRLF line breaks, and

^^

This put a well-needed smile on my face. Thanks.

> > such files can be edited on Windows, too.
> 
> Wouldn't this kind of .gitattributes setup solve the problem?
> 
> * -text
> *.sh   text eol=lf

If you look at the commit I mentioned, you will see examples where it
breaks down: when using Unix shell scripts *without* .sh file extension.
Most notable offender: GIT-VERSION-GEN.

Ciao,
Dscho

Gruß

2017-10-27 Thread Meinze Klaus Peter

'' In einer kurzen Einführung,

Ich bin ein Rechtsanwalt Meinze Klaus Peter, aus Deutschland, lebe
zurzeit in London, ich habe dir eine E-Mail über deine verstorbene
Familienangehörige verspätet. Ich habe keine Antwort von dir erhalten.
Die verstorbene Person ist eine Bürgerin deines Landes mit dem
gleichen Nachnamen mit dir ist er ein Goldexporteur hier in London,
mein Klient ist vor einigen Jahren mit seiner Familie gestorben, hat
sein Geschäft verlassen und riesige Summen des Gesamtgeldes hinterlegt

£ 5,7 Millionen GBP, bei  UBS Investment Bank hier in London bin ich
der persönliche Anwalt des verspäteten Kunden, und ich brauche Ihre
volle Zusammenarbeit, insofern wir das Geld von der Bank nehmen
können, bevor die Regierung es endgültig beschließt. in der Bank ist £
5,7 Millionen, I aber wird erklären, mehr Details, wenn ich von Ihnen
höre

   Ich brauche jetzt deine Antwort?

Re: [PATCH 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman

Hi Alex,

On Wed, 25 Oct 2017, Alex Vandiver wrote:

> [Johannes asked about the speedup when using --no-pretty]
>
> ...so a modest 8% speedup.  I note that those numbers are for a perl
> with JSON::XS installed; without it installed, the runtime is so long
> that I gave up waiting for it.

Very nice! AFAIR Git for Windows does *not* come with JSON::XS.

> Anyways, I'll put that in the commit message in the re-roll.

Thank you,
Johannes

Re: [PATCH 0/6] Create Git/Packet.pm

Hi Bryan,

On Thu, 26 Oct 2017, Bryan Turner wrote:

> On Thu, Oct 26, 2017 at 2:07 AM, Jacob Keller  wrote:
> > On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano  wrote:
> >> Johannes Schindelin  writes:
> >>
> >>> Note that the correct blib path starts with `C:\BuildAgent\_work` and
> >>> the line
> >>>
> >>>   use lib (split(/:/, $ENV{GITPERLLIB}));
> >>>
> >>> splits off the drive letter from the rest of the path. Obviously, this
> >>> fails to Do The Right Thing, and simply points to Yet Another Portability
> >>> Problem with Git's reliance on Unix scripting.
> >>
> >> In our C code, we have "#define PATH_SEP ';'", and encourage our
> >> code to be careful and use it.  Is there something similar for Perl
> >> scripts, I wonder.
> >>
> >> I notice that t/{t0202,t9000,t9700}/test.pl share the same
> >> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
> >> the non-platform convention to accomodate the use of split(/:/)
> >> certainly is a workaround, but it does feel dirty.
> >>
> >> It is hard to imagine that we were the first people who wants to
> >> split the value of a variable into a list, where the value is a list
> >> of paths, concatenated into a single string with a delimiter that
> >> may be platform specific.  I wonder if we are going against a best
> >> practice established in the Perl world, simply because we don't know
> >> about it (i.e. basically, it would say "don't split at a colon
> >> because not all world is Unix; use $this_module instead", similar to
> >> "don't split at a slash, use File::Spec instead to extract path
> >> components").
> >>
> >
> > I thought there was a way to do this in File::Spec, but that's only
> > for splitting regular paths, and not for splitting a list of paths
> > separated by ":" or ";"
> >
> > We probably should find a better solution to allow this to work with
> > windows style paths...? I know that python provides os.pathsep, but I
> > haven't seen an equivalent for perl yet.
> >
> > The Env[1] core modules suggests using $Config::Config{path_sep}[2]..
> > maybe we should be using this?
> 
> I was testing this recently on the Perl included with Git for Windows
> and it returns : for the path separator even though it's on Windows,
> so I don't think that would work. The Perl in Git for Windows seems to
> want UNIX-style inputs (something Dscho seemed to allude to in his
> response earlier.). I'm not sure why it's that way, but he probably
> knows.

MSYS2 Perl is essentially Cygwin's Perl ported over to MSYS2. And Cygwin
tries to keep everything as pseudo Unix as possible, to make it easier to
port software (if you think Git's source code is the only source code
woefully unprepared for semicolons as path separators, you just need to
buy me a few beers to hear plenty of war stories).

Ciao,
Dscho

Re: [PATCH 1/4] fsmonitor: Watch, and ask about, the top of the repo, not the CWD

Hi Alex,

On Wed, 25 Oct 2017, Alex Vandiver wrote:

> On Fri, 20 Oct 2017, Johannes Schindelin wrote:
> > This is super expensive, as it means a full-blown new process instead of
> > just a simple environment variable expansion.
> > 
> > The idea behind using `PWD` instead was that Git will already have done
> > all of the work of figuring out the top-level directory and switched to
> > there before calling the fsmonitor hook.
> 
> I'm not seeing that PWD has been at all altered.

No, PWD is not altered.

But a simple environment variable expansion (fast) was replaced by
spawning to `git rev-parse --show-top-level` (slow, ~60ms on Windows).

*That* is expensive.

Ciao,
Johannes

[PATCH v16 Part II 1/8] bisect--helper: `bisect_reset` shell function in C

Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
subcommand to `git bisect--helper` to call it from git-bisect.sh .

Using `bisect_reset` subcommand is a temporary measure to port shell
functions to C so as to use the existing test suite. As more functions
are ported, this subcommand would be retired but its implementation will
be called by some other method.

Note: --bisect-clean-state subcommand has not been retired as there are
still a function namely `bisect_start()` which still uses this
subcommand.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 

---
Hey,

This is the part 2 of the initial series[1] on bisect re-write. When I
submitted my patches of the part 1 of the bisect series, Ramsay informed
that he has been working on my previous patches and pointed me to his
patches. I have incorporated his changes into this series. Stephan did a
awesome job in reviewing my series and I have incorporated those changes
as well.

Ramsay's requested changes which have been incorporated:
 * bisect--helper: convert to struct object_id: I have took out the bits
   which were in this series and hand-picked them in this series.
 * bisect--helper: add a log_commit() helper function: This function
   might not seem to be very relevant in this part of the series, but it
   is going to be in future parts of the series as described in
   `188ea5855d93df` of Ramsay's git.git repo[2].
 * bisect--helper: reduce scope of a variable in `bisect_start`
 * bisect--helper: remove useless sub-expression in condition: I had
   already done this change as suggested by Stephan.
 * bisect--helper: set correct term from --term-new= option: I had
   already done this change as suggested by Stephan.
 * bisect--helper: remove redundant assignment to has_double_dash
 * bisect--helper: remove redundant goto's
 * bisect--helper: add some vertical whitespace
 * bisect--helper: fixup some coding style issues

Ramsay's changes which I had already done ([]-ed comments):
 * `bisect_write`: add a GIT_PATH_FUNC() removed earlier
 * `bisect_write`: lookup_commit_reference() now takes an object_id
 * `bisect_next_check`: addressed comments in Stephan's review and
   Junio's squash
 * `bisect_terms`: fixed up usage string and tested for --term-[good|old] and
   --term-[bad|new] in bisect-terms subcommand argument processing:
   instead of fixing up usage string, I just removed it as Stephan
   suggested.

Stephan's requested changes which have been incorporated:
 * Use goto's using `goto fail` and `goto finish`
 * Marking strings for translation
 * Stored res value and then returned it at the end
 * Implement set_terms() and free_terms() for proper memory handling for
   struct bisect_terms and fixed all leakages
 * Remove bisect_voc() function
 * Remove the bisect_term_usage string in bisect_terms()
 * Use return error() every where instead of die()
 * Improving readability of a message in bisect_terms()
 * bisect_start(): fix some typos, remove has_double_dash, join the two
   if branches, fix the bug when it should be terms->term_bad,
   decomposed it into smaller functions by introducing
   bisect_append_log()

A big thanks to Stephan and Ramsay for patiently reviewing my series and
helping me get this merged.

[1]:
https://public-inbox.org/git/0102015ecc65d695-22151d3b-752b-4c10-a3a3-b8ef52491664-000...@eu-west-1.amazonses.com/

[2]: git://repo.or.cz/git/raj.git
Regards,
Pranit Bauva
---
 builtin/bisect--helper.c | 49 +++-
 git-bisect.sh| 28 ++-
 2 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 35d2105f941c6..12754448f7b6a 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -3,15 +3,21 @@
 #include "parse-options.h"
 #include "bisect.h"
 #include "refs.h"
+#include "dir.h"
+#include "argv-array.h"
+#include "run-command.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
+static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
+static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms  "),
N_("git bisect--helper --bisect-clean-state"),
+   N_("git bisect--helper --bisect-reset []"),
NULL
 };
 
@@ -106,13 +112,48 @@ static void check_expected_revs(const char **revs, int 
rev_nr)
}
 }
 
+static int bisect_reset(const char *commit)
+{
+   struct strbuf branch = STRBUF_INIT;
+
+   if (!commit) {
+   if (strbuf_read_file(, git_path_bisect_start(), 0) < 1)
+

[PATCH v16 Part II 7/8] bisect--helper: `bisect_start` shell function partially in C

Reimplement the `bisect_start` shell function partially in C and add
`bisect-start` subcommand to `git bisect--helper` to call it from
git-bisect.sh .

The last part is not converted because it calls another shell function
`bisect_start` shell function will be completed after the `bisect_next`
shell function is ported in C.

Using `--bisect-start` subcommand is a temporary measure to port shell
function in C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by some
other methods.

Also introduce a method `bisect_append_log_quoted` to keep things short
and crisp.

Helped-by: Ramsay Jones 
Helped-by: Stephan Beyer 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 228 ++-
 git-bisect.sh| 132 +--
 2 files changed, 228 insertions(+), 132 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index ab0580ce0089a..4ac175c49e80c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -7,6 +7,7 @@
 #include "argv-array.h"
 #include "run-command.h"
 #include "prompt.h"
+#include "quote.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -14,6 +15,8 @@ static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, 
"BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
+static GIT_PATH_FUNC(git_path_head_name, "head-name")
+static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
@@ -24,6 +27,8 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
N_("git bisect--helper --bisect-next-check []  
"),
N_("git bisect--helper --bisect-terms [--term-good | --term-old | 
--term-bad | --term-new]"),
+   N_("git bisect--helper --bisect-start [--term-{old,good}= 
--term-{new,bad}=]"
+ "[--no-checkout] [ 
[...]] [--] [...]"),
NULL
 };
 
@@ -401,6 +406,217 @@ static int bisect_terms(struct bisect_terms *terms, const 
char **argv, int argc)
return 0;
 }
 
+static int bisect_append_log_quoted(const char **argv)
+{
+   int retval = 0;
+   FILE *fp = fopen(git_path_bisect_log(), "a");
+   struct strbuf orig_args = STRBUF_INIT;
+
+   if (!fp)
+   return -1;
+
+   if (fprintf(fp, "git bisect start") < 1)
+   goto fail;
+
+   sq_quote_argv(_args, argv, 0);
+   if (fprintf(fp, "%s\n", orig_args.buf) < 1)
+   goto fail;
+
+   goto finish;
+
+fail:
+   retval = -1;
+finish:
+   fclose(fp);
+   strbuf_release(_args);
+   return retval;
+}
+
+static int bisect_start(struct bisect_terms *terms, int no_checkout,
+   const char **argv, int argc)
+{
+   int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
+   int flags, pathspec_pos, retval = 0;
+   struct string_list revs = STRING_LIST_INIT_DUP;
+   struct string_list states = STRING_LIST_INIT_DUP;
+   struct strbuf start_head = STRBUF_INIT;
+   struct strbuf bisect_names = STRBUF_INIT;
+   struct object_id head_oid;
+   struct object_id oid;
+   const char *head;
+
+   if (is_bare_repository())
+   no_checkout = 1;
+
+   /*
+* Check for one bad and then some good revisions
+*/
+   for (i = 0; i < argc; i++) {
+   if (!strcmp(argv[i], "--")) {
+   has_double_dash = 1;
+   break;
+   }
+   }
+
+   for (i = 0; i < argc; i++) {
+   const char *arg = argv[i];
+   if (!strcmp(argv[i], "--")) {
+   break;
+   } else if (!strcmp(arg, "--no-checkout")) {
+   no_checkout = 1;
+   } else if (!strcmp(arg, "--term-good") ||
+!strcmp(arg, "--term-old")) {
+   must_write_terms = 1;
+   free((void *) terms->term_good);
+   terms->term_good = xstrdup(argv[++i]);
+   } else if (skip_prefix(arg, "--term-good=", ) ||
+  skip_prefix(arg, "--term-old=", )) {
+   must_write_terms = 1;
+   free((void *) terms->term_good);
+   terms->term_good = xstrdup(arg);
+   } else if (!strcmp(arg, "--term-bad") ||
+

[PATCH v16 Part II 6/8] bisect--helper: `get_terms` & `bisect_terms` shell function in C

Reimplement the `get_terms` and `bisect_terms` shell function in C and
add `bisect-terms` subcommand to `git bisect--helper` to call it from
git-bisect.sh .

Using `--bisect-terms` subcommand is a temporary measure to port shell
function in C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired but its implementation will
be called by some other methods.

Also use error() to report "no terms defined" and accordingly change the
test in t6030.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c| 69 +++--
 git-bisect.sh   | 35 ++-
 t/t6030-bisect-porcelain.sh |  2 +-
 3 files changed, 70 insertions(+), 36 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 0f9c3e63821b8..ab0580ce0089a 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -23,6 +23,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-write
 []"),
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
N_("git bisect--helper --bisect-next-check []  
"),
+   N_("git bisect--helper --bisect-terms [--term-good | --term-old | 
--term-bad | --term-new]"),
NULL
 };
 
@@ -344,6 +345,62 @@ static int bisect_next_check(const struct bisect_terms 
*terms,
return retval;
 }
 
+static int get_terms(struct bisect_terms *terms)
+{
+   struct strbuf str = STRBUF_INIT;
+   FILE *fp = NULL;
+   int res = 0;
+
+   fp = fopen(git_path_bisect_terms(), "r");
+   if (!fp)
+   goto fail;
+
+   free_terms(terms);
+   strbuf_getline_lf(, fp);
+   terms->term_bad = strbuf_detach(, NULL);
+   strbuf_getline_lf(, fp);
+   terms->term_good = strbuf_detach(, NULL);
+   goto finish;
+
+fail:
+   res = -1;
+finish:
+   if (fp)
+   fclose(fp);
+   strbuf_release();
+   return res;
+}
+
+static int bisect_terms(struct bisect_terms *terms, const char **argv, int 
argc)
+{
+   int i;
+
+   if (get_terms(terms))
+   return error(_("no terms defined"));
+
+   if (argc > 1)
+   return error(_("--bisect-term requires exactly one argument"));
+
+   if (argc == 0)
+   return !printf(_("Your current terms are %s for the old state\n"
+"and %s for the new state.\n"),
+terms->term_good, terms->term_bad);
+
+   for (i = 0; i < argc; i++) {
+   if (!strcmp(argv[i], "--term-good"))
+   printf(_("%s\n"), terms->term_good);
+   else if (!strcmp(argv[i], "--term-bad"))
+   printf(_("%s\n"), terms->term_bad);
+   else
+   error(_("BUG: invalid argument %s for 'git bisect 
terms'.\n"
+ "Supported options are: "
+ "--term-good|--term-old and "
+ "--term-bad|--term-new."), argv[i]);
+   }
+
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -354,7 +411,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
BISECT_RESET,
BISECT_WRITE,
CHECK_AND_SET_TERMS,
-   BISECT_NEXT_CHECK
+   BISECT_NEXT_CHECK,
+   BISECT_TERMS
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -374,6 +432,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("check and set terms in a bisection state"), 
CHECK_AND_SET_TERMS),
OPT_CMDMODE(0, "bisect-next-check", ,
 N_("check whether bad or good terms exist"), 
BISECT_NEXT_CHECK),
+   OPT_CMDMODE(0, "bisect-terms", ,
+N_("print out the bisect terms"), BISECT_TERMS),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -381,7 +441,7 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
struct bisect_terms terms;
 
argc = parse_options(argc, argv, prefix, options,
-git_bisect_helper_usage, 0);
+git_bisect_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
 
if (!cmdmode)
usage_with_options(git_bisect_helper_usage, options);
@@ -424,6 +484,11 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
set_terms(, argv[1], argv[0]);
res = bisect_next_check(, argc == 3 ?

[PATCH v16 Part II 5/8] bisect--helper: `bisect_next_check` shell function in C

Reimplement `bisect_next_check` shell function in C and add
`bisect-next-check` subcommand to `git bisect--helper` to call it from
git-bisect.sh .

`bisect_voc` shell function is no longer useful now and is replaced by
using a char *[] of "new|bad" and "good|old" values.

Using `--bisect-next-check` is a temporary measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired but its implementation will
be called by some other methods.

Helped-by: Stephan Beyer 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 91 +++-
 git-bisect.sh| 60 +++
 2 files changed, 94 insertions(+), 57 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 65abf8a70c6d9..0f9c3e63821b8 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -6,6 +6,7 @@
 #include "dir.h"
 #include "argv-array.h"
 #include "run-command.h"
+#include "prompt.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -21,6 +22,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-reset []"),
N_("git bisect--helper --bisect-write
 []"),
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
+   N_("git bisect--helper --bisect-next-check []  
"),
NULL
 };
 
@@ -44,6 +46,11 @@ static void set_terms(struct bisect_terms *terms, const char 
*bad,
terms->term_bad = xstrdup(bad);
 }
 
+static const char *voc[] = {
+   "bad|new",
+   "good|old"
+};
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -264,6 +271,79 @@ static int check_and_set_terms(struct bisect_terms *terms, 
const char *cmd)
return 0;
 }
 
+static int mark_good(const char *refname, const struct object_id *oid,
+int flag, void *cb_data)
+{
+   int *m_good = (int *)cb_data;
+   *m_good = 0;
+   return 1;
+}
+
+static int bisect_next_check(const struct bisect_terms *terms,
+const char *current_term)
+{
+   int missing_good = 1, missing_bad = 1, retval = 0;
+   const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
+   const char *good_glob = xstrfmt("%s-*", terms->term_good);
+
+   if (ref_exists(bad_ref))
+   missing_bad = 0;
+
+   for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
+(void *) _good);
+
+   if (!missing_good && !missing_bad)
+   goto finish;
+
+   if (!current_term)
+   goto fail;
+
+   if (missing_good && !missing_bad && current_term &&
+   !strcmp(current_term, terms->term_good)) {
+   char *yesno;
+   /*
+* have bad (or new) but not good (or old). We could bisect
+* although this is less optimum.
+*/
+   fprintf(stderr, _("Warning: bisecting only with a %s commit\n"),
+   terms->term_bad);
+   if (!isatty(0))
+   goto finish;
+   /*
+* TRANSLATORS: Make sure to include [Y] and [n] in your
+* translation. The program will only accept English input
+* at this point.
+*/
+   yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
+   if (starts_with(yesno, "N") || starts_with(yesno, "n"))
+   goto fail;
+
+   goto finish;
+   }
+   if (!is_empty_or_missing_file(git_path_bisect_start())) {
+   error(_("You need to give me at least one %s and "
+   "%s revision. You can use \"git bisect %s\" "
+   "and \"git bisect %s\" for that.\n"),
+   voc[0], voc[1], voc[0], voc[1]);
+   goto fail;
+   } else {
+   error(_("You need to start by \"git bisect start\". You "
+   "then need to give me at least one %s and %s "
+   "revision. You can use \"git bisect %s\" and "
+   "\"git bisect %s\" for that.\n"),
+   voc[1], voc[0], voc[1], voc[0]);
+   goto fail;
+   }
+   goto finish;
+
+fail:
+   retval = -1;
+finish:
+   free((void *) good_glob);
+   free((void *) bad_ref);
+   return retval;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -273,7 +353,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
CHECK_EXPECTED_REVS,

[PATCH v16 Part II 4/8] bisect--helper: `check_and_set_terms` shell function in C

Reimplement the `check_and_set_terms` shell function in C and add
`check-and-set-terms` subcommand to `git bisect--helper` to call it from
git-bisect.sh

Using `--check-and-set-terms` subcommand is a temporary measure to port
shell function in C so as to use the existing test suite. As more
functions are ported, this subcommand will be retired but its
implementation will be called by some other methods.

check_and_set_terms() sets and receives two global variables namely
TERM_GOOD and TERM_BAD in the shell script. Luckily the file BISECT_TERMS
also contains the value of those variables so its appropriate to evoke the
method get_terms() after calling the subcommand so that it retrieves the
value of TERM_GOOD and TERM_BAD from the file BISECT_TERMS. The two
global variables are passed as arguments to the subcommand.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 41 -
 git-bisect.sh| 36 
 2 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 6295f53c850a8..65abf8a70c6d9 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,6 +20,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-clean-state"),
N_("git bisect--helper --bisect-reset []"),
N_("git bisect--helper --bisect-write
 []"),
+   N_("git bisect--helper --bisect-check-and-set-terms  
 "),
NULL
 };
 
@@ -234,6 +235,35 @@ static int bisect_write(const char *state, const char *rev,
return retval;
 }
 
+static int check_and_set_terms(struct bisect_terms *terms, const char *cmd)
+{
+   int has_term_file = !is_empty_or_missing_file(git_path_bisect_terms());
+
+   if (one_of(cmd, "skip", "start", "terms", NULL))
+   return 0;
+
+   if (has_term_file && strcmp(cmd, terms->term_bad) &&
+   strcmp(cmd, terms->term_good))
+   return error(_("Invalid command: you're currently in a "
+   "%s/%s bisect"), terms->term_bad,
+   terms->term_good);
+
+   if (!has_term_file) {
+   if (one_of(cmd, "bad", "good", NULL)) {
+   free_terms(terms);
+   set_terms(terms, "bad", "good");
+   return write_terms(terms->term_bad, terms->term_good);
+   }
+   else if (one_of(cmd, "new", "old", NULL)) {
+   free_terms(terms);
+   set_terms(terms, "new", "old");
+   return write_terms(terms->term_bad, terms->term_good);
+   }
+   }
+
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -242,7 +272,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
BISECT_CLEAN_STATE,
CHECK_EXPECTED_REVS,
BISECT_RESET,
-   BISECT_WRITE
+   BISECT_WRITE,
+   CHECK_AND_SET_TERMS
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -258,6 +289,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("reset the bisection state"), BISECT_RESET),
OPT_CMDMODE(0, "bisect-write", ,
 N_("update the refs according to the bisection state 
and may write it to BISECT_LOG"), BISECT_WRITE),
+   OPT_CMDMODE(0, "check-and-set-terms", ,
+N_("check and set terms in a bisection state"), 
CHECK_AND_SET_TERMS),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -296,6 +329,12 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
set_terms(, argv[3], argv[2]);
res = bisect_write(argv[0], argv[1], , nolog);
break;
+   case CHECK_AND_SET_TERMS:
+   if (argc != 3)
+   return error(_("--check-and-set-terms requires 3 
arguments"));
+   set_terms(, argv[2], argv[1]);
+   res = check_and_set_terms(, argv[0]);
+   break;
default:
return error("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 1eab94ec943a1..0447b73aa588b 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -238,7 +238,8 @@ bisect_skip() {
 bisect_state() {
bisect_autostart
state=$1
-   check_and_set_terms $state
+   git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD ||

[PATCH v16 Part II 3/8] wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()

is_empty_file() can help to refactor a lot of code. This will be very
helpful in porting "git bisect" to C.

Suggested-by: Torsten Bögershausen 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/am.c | 20 ++--
 cache.h  |  3 +++
 wrapper.c| 13 +
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 4b6f1534f8840..2afaaabcdbecf 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -34,22 +34,6 @@
 #include "packfile.h"
 
 /**
- * Returns 1 if the file is empty or does not exist, 0 otherwise.
- */
-static int is_empty_file(const char *filename)
-{
-   struct stat st;
-
-   if (stat(filename, ) < 0) {
-   if (errno == ENOENT)
-   return 1;
-   die_errno(_("could not stat %s"), filename);
-   }
-
-   return !st.st_size;
-}
-
-/**
  * Returns the length of the first line of msg.
  */
 static int linelen(const char *msg)
@@ -1298,7 +1282,7 @@ static int parse_mail(struct am_state *state, const char 
*mail)
goto finish;
}
 
-   if (is_empty_file(am_path(state, "patch"))) {
+   if (is_empty_or_missing_file(am_path(state, "patch"))) {
printf_ln(_("Patch is empty."));
die_user_resolve(state);
}
@@ -1880,7 +1864,7 @@ static void am_run(struct am_state *state, int resume)
resume = 0;
}
 
-   if (!is_empty_file(am_path(state, "rewritten"))) {
+   if (!is_empty_or_missing_file(am_path(state, "rewritten"))) {
assert(state->rebasing);
copy_notes_for_rebase(state);
run_post_rewrite_hook(state);
diff --git a/cache.h b/cache.h
index 25adcf681531c..f05056a2d7fa1 100644
--- a/cache.h
+++ b/cache.h
@@ -1958,4 +1958,7 @@ void sleep_millisec(int millisec);
  */
 void safe_create_dir(const char *dir, int share);
 
+/* Return 1 if the file is empty or does not exists, 0 otherwise. */
+extern int is_empty_or_missing_file(const char *filename);
+
 #endif /* CACHE_H */
diff --git a/wrapper.c b/wrapper.c
index 61aba0b5c1bec..21c0fee2db459 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -690,3 +690,16 @@ int xgethostname(char *buf, size_t len)
buf[len - 1] = 0;
return ret;
 }
+
+int is_empty_or_missing_file(const char *filename)
+{
+   struct stat st;
+
+   if (stat(filename, ) < 0) {
+   if (errno == ENOENT)
+   return 1;
+   die_errno(_("could not stat %s"), filename);
+   }
+
+   return !st.st_size;
+}

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

[PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

Reimplement the `bisect_write` shell function in C and add a
`bisect-write` subcommand to `git bisect--helper` to call it from
git-bisect.sh

Using `--bisect-write` subcommand is a temporary measure to port shell
function in C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired but its implementation will
be called by some other methods.

Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
from the global shell script thus we need to pass it to the subcommand
using the arguments. We then store them in a struct bisect_terms and
pass the memory address around functions.

Add a log_commit() helper function to write the contents of the commit message
header to a file which will be re-used in future parts of the code as
well.

Also introduce a function free_terms() to free the memory of `struct
bisect_terms` and set_terms() to set the values of members in `struct
bisect_terms`.

Helped-by: Ramsay Jones 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 107 +--
 git-bisect.sh|  25 ++-
 2 files changed, 108 insertions(+), 24 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 12754448f7b6a..6295f53c850a8 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -12,15 +12,37 @@ static GIT_PATH_FUNC(git_path_bisect_expected_rev, 
"BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
+static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms  "),
N_("git bisect--helper --bisect-clean-state"),
N_("git bisect--helper --bisect-reset []"),
+   N_("git bisect--helper --bisect-write
 []"),
NULL
 };
 
+struct bisect_terms {
+   const char *term_good;
+   const char *term_bad;
+};
+
+static void free_terms(struct bisect_terms *terms)
+{
+   if (!terms->term_good)
+   free((void *) terms->term_good);
+   if (!terms->term_bad)
+   free((void *) terms->term_bad);
+}
+
+static void set_terms(struct bisect_terms *terms, const char *bad,
+ const char *good)
+{
+   terms->term_good = xstrdup(good);
+   terms->term_bad = xstrdup(bad);
+}
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -146,6 +168,72 @@ static int bisect_reset(const char *commit)
return bisect_clean_state();
 }
 
+static void log_commit(FILE *fp, char *fmt, const char *state,
+  struct commit *commit)
+{
+   struct pretty_print_context pp = {0};
+   struct strbuf commit_msg = STRBUF_INIT;
+   char *label = xstrfmt(fmt, state);
+
+   format_commit_message(commit, "%s", _msg, );
+
+   fprintf(fp, "# %s: [%s] %s\n", label, oid_to_hex(>object.oid),
+   commit_msg.buf);
+
+   strbuf_release(_msg);
+   free(label);
+}
+
+static int bisect_write(const char *state, const char *rev,
+   const struct bisect_terms *terms, int nolog)
+{
+   struct strbuf tag = STRBUF_INIT;
+   struct object_id oid;
+   struct commit *commit;
+   FILE *fp = NULL;
+   int retval = 0;
+
+   if (!strcmp(state, terms->term_bad)) {
+   strbuf_addf(, "refs/bisect/%s", state);
+   } else if (one_of(state, terms->term_good, "skip", NULL)) {
+   strbuf_addf(, "refs/bisect/%s-%s", state, rev);
+   } else {
+   error(_("Bad bisect_write argument: %s"), state);
+   goto fail;
+   }
+
+   if (get_oid(rev, )) {
+   error(_("couldn't get the oid of the rev '%s'"), rev);
+   goto fail;
+   }
+
+   if (update_ref(NULL, tag.buf, , NULL, 0,
+  UPDATE_REFS_MSG_ON_ERR))
+   goto fail;
+
+   fp = fopen(git_path_bisect_log(), "a");
+   if (!fp) {
+   error_errno(_("couldn't open the file '%s'"), 
git_path_bisect_log());
+   goto fail;
+   }
+
+   commit = lookup_commit_reference();
+   log_commit(fp, "%s", state, commit);
+
+   if (!nolog)
+   fprintf(fp, "git bisect %s %s\n", state, rev);
+
+   goto finish;
+
+fail:
+   retval = -1;
+finish:
+   if (fp)
+   fclose(fp);
+   strbuf_release();
+   return retval;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -153,9 +241,10 @@ int cmd_bisect__helper(int argc,

[PATCH v16 Part II 8/8] t6030: make various test to pass GETTEXT_POISON tests

Signed-off-by: Pranit Bauva 
---
 t/t6030-bisect-porcelain.sh | 120 ++--
 1 file changed, 60 insertions(+), 60 deletions(-)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 55835ee4a4715..f9e61c6540e57 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -133,21 +133,21 @@ test_expect_success 'bisect reset removes bisect state 
after --no-checkout' '
 
 test_expect_success 'bisect start: back in good branch' '
git branch > branch.output &&
-   grep "* other" branch.output > /dev/null &&
+   test_i18ngrep "* other" branch.output > /dev/null &&
git bisect start $HASH4 $HASH1 -- &&
git bisect good &&
git bisect start $HASH4 $HASH1 -- &&
git bisect bad &&
git bisect reset &&
git branch > branch.output &&
-   grep "* other" branch.output > /dev/null
+   test_i18ngrep "* other" branch.output > /dev/null
 '
 
 test_expect_success 'bisect start: no ".git/BISECT_START" created if junk rev' 
'
git bisect reset &&
test_must_fail git bisect start $HASH4 foo -- &&
git branch > branch.output &&
-   grep "* other" branch.output > /dev/null &&
+   test_i18ngrep "* other" branch.output > /dev/null &&
test_must_fail test -e .git/BISECT_START
 '
 
@@ -158,14 +158,14 @@ test_expect_success 'bisect start: existing 
".git/BISECT_START" not modified if
test_must_fail git bisect start $HASH4 foo -- &&
git branch > branch.output &&
test_i18ngrep "* (no branch, bisect started on other)" branch.output > 
/dev/null &&
-   test_cmp saved .git/BISECT_START
+   test_i18ncmp saved .git/BISECT_START
 '
 test_expect_success 'bisect start: no ".git/BISECT_START" if mistaken rev' '
git bisect start $HASH4 $HASH1 -- &&
git bisect good &&
test_must_fail git bisect start $HASH1 $HASH4 -- &&
git branch > branch.output &&
-   grep "* other" branch.output > /dev/null &&
+   test_i18ngrep "* other" branch.output > /dev/null &&
test_must_fail test -e .git/BISECT_START
 '
 
@@ -174,7 +174,7 @@ test_expect_success 'bisect start: no ".git/BISECT_START" 
if checkout error' '
test_must_fail git bisect start $HASH4 $HASH1 -- &&
git branch &&
git branch > branch.output &&
-   grep "* other" branch.output > /dev/null &&
+   test_i18ngrep "* other" branch.output > /dev/null &&
test_must_fail test -e .git/BISECT_START &&
test -z "$(git for-each-ref "refs/bisect/*")" &&
git checkout HEAD hello
@@ -189,7 +189,7 @@ test_expect_success 'bisect skip: successful result' '
git bisect start $HASH4 $HASH1 &&
git bisect skip &&
git bisect bad > my_bisect_log.txt &&
-   grep "$HASH2 is the first bad commit" my_bisect_log.txt
+   test_i18ngrep "$HASH2 is the first bad commit" my_bisect_log.txt
 '
 
 # $HASH1 is good, $HASH4 is bad, we skip $HASH3 and $HASH2
@@ -200,11 +200,11 @@ test_expect_success 'bisect skip: cannot tell between 3 
commits' '
git bisect start $HASH4 $HASH1 &&
git bisect skip &&
test_expect_code 2 git bisect skip >my_bisect_log.txt &&
-   grep "first bad commit could be any of" my_bisect_log.txt &&
+   test_i18ngrep "first bad commit could be any of" my_bisect_log.txt &&
! grep $HASH1 my_bisect_log.txt &&
-   grep $HASH2 my_bisect_log.txt &&
-   grep $HASH3 my_bisect_log.txt &&
-   grep $HASH4 my_bisect_log.txt
+   test_i18ngrep $HASH2 my_bisect_log.txt &&
+   test_i18ngrep $HASH3 my_bisect_log.txt &&
+   test_i18ngrep $HASH4 my_bisect_log.txt
 '
 
 # $HASH1 is good, $HASH4 is bad, we skip $HASH3
@@ -216,11 +216,11 @@ test_expect_success 'bisect skip: cannot tell between 2 
commits' '
git bisect start $HASH4 $HASH1 &&
git bisect skip &&
test_expect_code 2 git bisect good >my_bisect_log.txt &&
-   grep "first bad commit could be any of" my_bisect_log.txt &&
+   test_i18ngrep "first bad commit could be any of" my_bisect_log.txt &&
! grep $HASH1 my_bisect_log.txt &&
! grep $HASH2 my_bisect_log.txt &&
-   grep $HASH3 my_bisect_log.txt &&
-   grep $HASH4 my_bisect_log.txt
+   test_i18ngrep $HASH3 my_bisect_log.txt &&
+   test_i18ngrep $HASH4 my_bisect_log.txt
 '
 
 # $HASH1 is good, $HASH4 is both skipped and bad, we skip $HASH3
@@ -235,11 +235,11 @@ test_expect_success 'bisect skip: with commit both bad 
and skipped' '
git bisect good $HASH1 &&
git bisect skip &&
test_expect_code 2 git bisect good >my_bisect_log.txt &&
-   grep "first bad commit could be any of" my_bisect_log.txt &&
+   test_i18ngrep "first bad commit could be any of" my_bisect_log.txt &&
! grep $HASH1 my_bisect_log.txt &&
-   ! grep $HASH2 my_bisect_log.txt &&
-   grep $HASH3 my_bisect_log.txt &&
-   grep $HASH4

Re: [PATCH 0/6] Create Git/Packet.pm

Hi Junio,

On Thu, 26 Oct 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Note that the correct blib path starts with `C:\BuildAgent\_work` and
> > the line
> >
> > use lib (split(/:/, $ENV{GITPERLLIB}));
> >
> > splits off the drive letter from the rest of the path. Obviously, this
> > fails to Do The Right Thing, and simply points to Yet Another Portability
> > Problem with Git's reliance on Unix scripting.
> 
> In our C code, we have "#define PATH_SEP ';'", and encourage our
> code to be careful and use it.  Is there something similar for Perl
> scripts, I wonder.
> 
> I notice that t/{t0202,t9000,t9700}/test.pl share the same
> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
> the non-platform convention to accomodate the use of split(/:/)
> certainly is a workaround, but it does feel dirty.

It is not only dirty, it *still* does not work with that workaround: Note
that GITPERLLIB is *also* set in the bin-wrappers/*...

And even then, it does not work: somewhere along the way, the path is
converted to a Windows path with backslashes, and for some reason MSYS2
Perl fails to handle that.

The best workaround I found in Git's source code (yes, it took me 2h to
investigate this littly problem, but at least I got an in-depth
understanding of the issue) was to pass BLIB_DIR instead, and not perform
a split but generate the two paths to include in Perl instead. Of course,
that would break out-of-tree usage of GITPERLLIB...

That's why I settled on the out-of-tree workaround that Windows
contributors will have to somehow figure out (as if it was not hard enough
already to contribute to Git for Windows...).

> It is hard to imagine that we were the first people who wants to
> split the value of a variable into a list, where the value is a list
> of paths, concatenated into a single string with a delimiter that
> may be platform specific.

No, the test suite has plenty of use cases for that. It usually works.

The problem is that t0021 contains very complex code that goes back and
forth between the C layer and the scripted layer. At one stage, the
pseudo-Unix paths are converted to Windows paths, with drive prefix and
backslashes, separated by semicolons. And somewhere along the lines, this
cannot be converted back.

I *think* that it happens when the bin-wrapper for git.exe is executed
from inside Git itself, or some such.

> I wonder if we are going against a best practice established in the Perl
> world, simply because we don't know about it (i.e. basically, it would
> say "don't split at a colon because not all world is Unix; use
> $this_module instead", similar to "don't split at a slash, use
> File::Spec instead to extract path components").

We go against best practices by having crucial parts of Git implemented as
Perl scripts. But you knew that ;-)

Ciao,
Dscho

Re: [PATCH] dir: allow exclusions from blob in addition to file

2017-10-27 Thread Jeff Hostetler




On 10/26/2017 9:20 PM, Junio C Hamano wrote:

Jeff Hostetler  writes:


From: Jeff Hostetler 

Refactor add_excludes() to separate the reading of the
exclude file into a buffer and the parsing of the buffer
into exclude_list items.

Add add_excludes_from_blob_to_list() to allow an exclude
file be specified with an OID without assuming a local
worktree or index exists.

Refactor read_skip_worktree_file_from_index() and add
do_read_blob() to eliminate duplication of preliminary
processing of blob contents.

Signed-off-by: Jeff Hostetler 
---


Yeah, with a separate do_read_blob() helper, this one looks a lot
easier to follow, at least to me---as the author, you might find the
earlier one just as easy, I suspect, though ;-)

Thanks.  Will queue.



Yeah, I think the net result is better and easier to follow.
Thanks,
Jeff

Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH

Hi Peff,

On Wed, 25 Oct 2017, Jeff King wrote:

> On Wed, Oct 25, 2017 at 11:35:44PM +0200, Johannes Schindelin wrote:
> 
> > > > Or alternatively we could prefix the assignment by
> > > > 
> > > > test -n "$TEST_SHELL_PATH" ||
> > > > 
> > > > or use the pattern
> > > > 
> > > > TEST_SHELL_PATH="${TEST_SHELL_PATH:-[...]}"
> > > 
> > > I'm not quite sure what this is fixing.  Is there a case where we
> > > wouldn't have TEST_SHELL_PATH set when running the tests? I think there
> > > are already other bits that assume that "make" has been run (including
> > > the existing reference to $SHELL_PATH, I think).
> > 
> > The way I read your patch, setting the environment variable differnently
> > at test time than at build time would be ignored: GIT-BUILD-OPTIONS is
> > sourced and would override whatever you told the test suite to use.
> > 
> > I guess it does not really matter all that much in practice.
> 
> Right. I find that behavior mildly irritating at times, but it's
> consistent with other items like NO_PERL, etc. E.g., you cannot do:
> 
>   make NO_PERL=
>   cd t
>   NO_PERL=Nope ./t3701-*
> 
> and disable perl. It's testing what got built.

The difference between NO_PERL and TEST_SHELL_PATH, of course, is that
NO_PERL affects the build and the test phase, while TEST_SHELL_PATH really
has no impact whatsoever on the build phase.

Ciao,
Dscho

clarify documentation of leading "**" in gitignore(5) man page

2017-10-27 Thread David Prager Branner

I've long been confused by something in the man page for gitignore. I think
it's unclear and I'd like to propose a change. The passage is this (source at
https://git.kernel.org/pub/scm/git/git-manpages.git/tree/man5/gitignore.5):

Two consecutive asterisks ("**") in patterns matched against full
pathname may have special meaning:

o A leading "**" followed by a slash means match in all directories.
For example, "**/foo" matches file or directory "foo" anywhere, the
same as pattern "foo". "**/foo/bar" matches file or directory "bar"
anywhere that is directly under directory "foo".

...

In the first paragraph, it would be clearer to specify:

Two consecutive asterisks ("**"), in patterns matched against a full
pathname, are a wildcard representing some arbitrary number of nested
directories within that pathname:

In the second paragraph, I suggest removing the first pattern, "**/foo", since it is completely
redundant. There are no circumstances when "**/foo" is necessary in place of plain "foo",
and its presence muddies discussion.

Leading "**" may represent nested directories in a pathname, but is useful only
before a _pattern_ containing nested directories. I suggest making that explicit. Here is
how I propose to rewrite it:

o A leading "**", followed by a slash and a pattern containing nested
directories, means match that pattern in all pathnames.
For example, "**/foo/bar" matches file or directory "bar"
anywhere that is directly under directory "foo". It would match
"foo/bar", "other/foo/bar", "src/foo/bar/scram/gravy".

Thanks.

- dpb

[Bug Report] [includeIf] does not parse case insensitive -> case sensitive symlink gitdir

2017-10-27 Thread Adrian Kappel

Hello all, not sure if the issue I've come across is a known bug or
addressable, but wanted to report it just in-case.


** Summary
--
Using the [includeIf] configuration statement with a symlink'd gitdir
will not work if the symlink is on a case insensitive volume and the
location it references is on a case sensitive volume.

** Steps to reproduce
--
1. Create symlink (case insensitive -> case sensitive):
/Users/usera/dev -> /Volumes/CaseSensitive/dev
2. Create two files: .gitignore and .gitignore-work, both stored in
/Users/usera/

.gitconfig
-
[user]
  name = First Last

  [includeIf "gitdir:~/dev/work"]
path = .gitconfig-work

.gitconfig-work

[user]
  email = em...@address.com

3. cd into a subfolder of ~/dev/work that has been git initialized.
Let's say ~/dev/work/repo
4. Run git config --includes user.email
5. See that nothing is output from the command
6. Update the [includeIf] statement in .gitconfig to be the real
location i.e. "gitdir:/Volumes/CaseSensitive/dev/work/repo"
7. Rerun the command from [4]
8. See that em...@address.com is output from the command

** Other variations that were not tested
--
- symlink on case sensitive volume referencing a location on a case
insensitive volume

** Environment Information
--
git --version: 2.14.1
OS: macOS Sierra 10.12.6


If a fix is not feasible or likely to be implemented, I would
recommend that we update the git site's documentation to reflect this
gotcha. After verification of course.

Best,
Adrian Kappel
akappel

Re: Why does fetch-pack not works over http?

2017-10-27 Thread Andreas Schwab

On Okt 27 2017, Alvaro del Castillo  wrote:

> We're wondering why "fetch-pack" (when is running from the command
> line) doesn't handle "https://; protocol. It only works with "git://".
>
> For instance, this doesn't work:
>
> $ git fetch-pack https://github.com/git/git refs/heads/master
> fatal: I don't handle protocol 'https'
>
> while this does:
>
> $ git fetch-pack git://github.com/git/git refs/heads/master
>
> The funny thing is that under the hood, "fetch" calls "fetch-pack"
> using "https" procotol. Example of a trace below:
>
> 12:03:07.512558 git.c:344   trace: built-in: git 'fetch-
> pack' '--stateless-rpc' '--stdin' '--lock-pack' '--thin' 'https://githu
> b.com/git/git/'

With --stateless-rpc, fetch-pack doesn't do the connect itself, but
expects the caller having set up a pipe to it.  The URL is then actually
ignored.

Andreas.

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

Re: [git-for-windows] Git for Windows v2.15.0-rc prerelease, was Re: [ANNOUNCE] Git v2.15.0-rc2

2017-10-27 Thread Lars Schneider


> On 27 Oct 2017, at 14:11, Lars Schneider  wrote:
> 
> 
>> On 21 Oct 2017, at 00:22, Johannes Schindelin  
>> wrote:
>> 
>> Hi team,
>> 
>> [cutting linux-kernel]
>> 
>> On Fri, 20 Oct 2017, Junio C Hamano wrote:
>> 
>>> A release candidate Git v2.15.0-rc2 is now available for testing
>>> at the usual places.
>> 
>> The Git for Windows equivalent is now available from
>> 
>>   https://github.com/git-for-windows/git/releases/tag/v2.15.0-rc2.windows.1
> 
> Hi Dscho,
> 
> I just tested RC2 on Windows and I don't see my "Filtering content:" 
> output if I clone a Git repository with Git LFS files (and Git LFS 
> 2.3.3+ installed).
> 
> The feature was introduced in the following commit which is be part of 
> your RC2 build commit (b7f8941):
> https://github.com/git/git/commit/52f1d62eb44faf569edca360ec9af9ddd4045fe0
> 
> On macOS everything works as expcted with RC2:
>...
>remote: Total 15012 (delta 0), reused 0 (delta 0), pack-reused 15012
>Receiving objects: 100% (15012/15012), 2.02 MiB | 753.00 KiB/s, done.
>Filtering content:  43% (6468/15000), 33.30 KiB | 0 bytes/s
>...
> 
> Do you, or other Windows experts, spot something in the commit linked
> above that could cause trouble on Windows?

Well, it turns out the output works for my real life repos but not for
my Git LFS testing repo.

git clone https://github.com/larsxschneider/lfstest-manyfiles

... prints the filtering content output on macOS but not on Windows.
The progress function has some delay feature that suppresses the output
if it is only shown for a second or something. However, in this test case
the output should be visible for several seconds at least...
I am still puzzled.

- Lars

Re: [git-for-windows] Git for Windows v2.15.0-rc prerelease, was Re: [ANNOUNCE] Git v2.15.0-rc2

2017-10-27 Thread Lars Schneider


> On 21 Oct 2017, at 00:22, Johannes Schindelin  
> wrote:
> 
> Hi team,
> 
> [cutting linux-kernel]
> 
> On Fri, 20 Oct 2017, Junio C Hamano wrote:
> 
>> A release candidate Git v2.15.0-rc2 is now available for testing
>> at the usual places.
> 
> The Git for Windows equivalent is now available from
> 
>https://github.com/git-for-windows/git/releases/tag/v2.15.0-rc2.windows.1

Hi Dscho,

I just tested RC2 on Windows and I don't see my "Filtering content:" 
output if I clone a Git repository with Git LFS files (and Git LFS 
2.3.3+ installed).

The feature was introduced in the following commit which is be part of 
your RC2 build commit (b7f8941):
https://github.com/git/git/commit/52f1d62eb44faf569edca360ec9af9ddd4045fe0

On macOS everything works as expcted with RC2:
...
remote: Total 15012 (delta 0), reused 0 (delta 0), pack-reused 15012
Receiving objects: 100% (15012/15012), 2.02 MiB | 753.00 KiB/s, done.
Filtering content:  43% (6468/15000), 33.30 KiB | 0 bytes/s
...

Do you, or other Windows experts, spot something in the commit linked
above that could cause trouble on Windows?

Thanks,
Lars

Why does fetch-pack not works over http?

2017-10-27 Thread Alvaro del Castillo

Dear all,

We're wondering why "fetch-pack" (when is running from the command
line) doesn't handle "https://; protocol. It only works with "git://".

For instance, this doesn't work:

$ git fetch-pack https://github.com/git/git refs/heads/master
fatal: I don't handle protocol 'https'

while this does:

$ git fetch-pack git://github.com/git/git refs/heads/master

The funny thing is that under the hood, "fetch" calls "fetch-pack"
using "https" procotol. Example of a trace below:

12:03:07.512558 git.c:344   trace: built-in: git 'fetch-
pack' '--stateless-rpc' '--stdin' '--lock-pack' '--thin' 'https://githu
b.com/git/git/'

Cheers
-- 
Alvaro del Castillo San Félix
a...@bitergia.com - Chief Technical Officer (CTO)
http://www.bitergia.com
"Software metrics for your peace of mind"

[PATCH] diff: fix lstat() error handling in diff_populate_filespec()

2017-10-27 Thread Andrey Okoshkin

Add lstat() error handling not only for ENOENT case.
Otherwise uninitialised 'struct stat st' variable is used later in case of
lstat() non-ENOENT failure which leads to processing of rubbish values of
file mode ('S_ISLNK(st.st_mode)' check) or size ('xsize_t(st.st_size)').

Signed-off-by: Andrey Okoshkin 
---
Hello,

I've injected a fault to git binary with the internal tool for fault tolerance
evaluation: lstat() returns '-1' and errno is set to 'EACCES' at
diff_populate_filespec git/diff.c:2850.
In a real life it's very difficult to reproduce such behaviour.
I'm not sure why only ENOENT error of lstat() is considered as an error but 
passing
by other errno values leads to reading of uninitialized 'struct stat st' 
variable.
It means that the populated 'diff_filespec' structure may be incorrectly filled.

Also diff_populate_filespec() result is not checked at 
diff_filespec_is_binary() but
it seems OK there.

Best regards,
Andrey

 diff.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index 8406a8324..d737a78a1 100644
--- a/diff.c
+++ b/diff.c
@@ -2848,14 +2848,12 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
int fd;
 
if (lstat(s->path, ) < 0) {
-   if (errno == ENOENT) {
-   err_empty:
-   err = -1;
-   empty:
-   s->data = (char *)"";
-   s->size = 0;
-   return err;
-   }
+   err_empty:
+   err = -1;
+   empty:
+   s->data = (char *)"";
+   s->size = 0;
+   return err;
}
s->size = xsize_t(st.st_size);
if (!s->size)
-- 
2.14.3

What's cooking in git.git (Oct 2017, #06; Fri, 27)

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

The topics that are cooking in 'next' that are not urgent fixes are
classified as "Will cook in 'next'", and will not graduate to
'master' until the final.

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

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

--
[Graduated to "master"]

* mh/ref-locking-fix (2017-10-25) 2 commits
  (merged to 'next' on 2017-10-26 at 1f1091ad64)
 + files_transaction_prepare(): fix handling of ref lock failure
 + t1404: add a bunch of tests of D/F conflicts

 Transactions to update multiple references that involves a deletion
 was quite broken in an error codepath and did not abort everything
 correctly.

--
[New Topics]

* js/submodule-in-excluded (2017-10-26) 1 commit
  (merged to 'next' on 2017-10-26 at 2a262e6a0b)
 + status: do not get confused by submodules in excluded directories

 "git status --ignored -u" did not stop at a working tree of a
 separate project that is embedded in an ignored directory and
 listed files in that other project, instead of just showing the
 directory itself as ignored.

 Will cook in 'next'.


* ao/path-use-xmalloc (2017-10-25) 1 commit
  (merged to 'next' on 2017-10-26 at 4cc04083fb)
 + path.c: use xmalloc() in add_to_trie()

 A possible oom error is now caught as a fatal error, instead of
 continuing and dereferencing NULL.

 Will merge to 'master'.


* tb/complete-checkout (2017-10-25) 1 commit
  (merged to 'next' on 2017-10-26 at beeaf5b00c)
 + completion: add remaining flags to checkout

 Command line completion (in contrib/) update.

 Will cook in 'next'.


* jc/ignore-cr-at-eol (2017-10-26) 2 commits
 - diff: --ignore-cr-at-eol
 - xdiff: reassign xpparm_t.flags bits

 The "diff" family of commands learned to ignore differences in
 carriage return at the end of line.

 Just a lunch-time hack.
 Lacks tests, docs and proper log message.


* sb/diff-color-moved-use-xdl-recmatch (2017-10-26) 2 commits
  (merged to 'next' on 2017-10-26 at 6711f24498)
 + diff.c: get rid of duplicate implementation
 + xdiff-interface: export comparing and hashing strings
 (this branch uses jk/diff-color-moved-fix.)

 Instead of using custom line comparison and hashing functions to
 implement "moved lines" coloring in the diff output, use the pair
 of these functions from lower-layer xdiff/ code.

 Will cook in 'next'.


* jh/dir-add-exclude-from-blob (2017-10-27) 1 commit
 - dir: allow exclusions from blob in addition to file

 The code to read exclusion list from a blob, which is used when the
 .gitignore file is outside a sparse checkout area, has been
 refactored so that other codepath can later use it to do the same
 outside the context of "sparse checkout".


* mh/avoid-rewriting-packed-refs (2017-10-27) 2 commits
 - files-backend: don't rewrite the `packed-refs` file unnecessarily
 - t1409: check that `packed-refs` is not rewritten unnecessarily

 Recent update to the refs infrastructure implementation started
 rewriting packed-refs file more often than before; this has been
 optimized again for most trivial cases.


* sb/rev-parse-show-superproject-root (2017-10-27) 1 commit
  (merged to 'next' on 2017-10-27 at f9a0520cec)
 + docs: fix formatting of rev-parse's --show-superproject-working-tree

 Doc markup fix.

 Will merge to 'master'.


* sg/rev-list-doc-reorder-fix (2017-10-27) 1 commit
  (merged to 'next' on 2017-10-27 at caf49859d7)
 + rev-list-options.txt: use correct directional reference

 Doc flow fix.

 Will merge to 'master'.

--
[Stalled]

* mk/use-size-t-in-zlib (2017-08-10) 1 commit
 . zlib.c: use size_t for size

 The wrapper to call into zlib followed our long tradition to use
 "unsigned long" for sizes of regions in memory, which have been
 updated to use "size_t".

 Needs resurrecting by making sure the fix is good and still applies
 (or adjusted to today's codebase).


* mg/status-in-progress-info (2017-05-10) 2 commits
 - status --short --inprogress: spell it as --in-progress
 - status: show in-progress info for short status

 "git status" learns an option to report various operations
 (e.g. "merging") that the user is in the middle of.

 cf. 


* nd/worktree-move (2017-04-20) 6 commits
 - worktree remove: new command
 - worktree move: refuse to move worktrees with submodules
 - worktree move: accept destination as directory
 - worktree move: new command
 - worktree.c: add update_worktree_location()
 - worktree.c: add validate_worktree()

 "git worktree" learned move and remove subcommands.

 Expecting a reroll.
 cf.

Re: [PATCH] tag: add tag.gpgSign config option to force all tags be GPG-signed

2017-10-27 Thread Mkrtchyan, Tigran

Hi Jonathan,

I can't disagree with you - the right solution is to fix the build/release 
process to support
signed tagging. It's just too many of them to fix: jenkins, maven, IDE, etc. My 
naive assumption
was that if a tool (git) has a switch to enable some functionality, why not 
have a possibility
to make it default.

You can put this change on hold and re-consider it if more people will need 
such functionality.

Thanks,
   Tigran.

- Original Message -
> From: "Jonathan Nieder" 
> To: "Tigran Mkrtchyan" 
> Cc: "git" 
> Sent: Thursday, October 26, 2017 11:33:37 PM
> Subject: Re: [PATCH] tag: add tag.gpgSign config option to force all tags be 
> GPG-signed

> Hi again,
> 
> Mkrtchyan, Tigran wrote:
>> Jonathan Nieder wrote:
>>> Tigran Mkrtchyan wrote:
> 
 In some workflows we have no control on how git command is executed,
 however a signed tags are required.
>>>
>>> Don't leave me hanging: this leaves me super curious.  Can you tell me
>>> more about these workflows?
>>
>> Well, this is a build/release process where we can't pass additional
>> command line options to git. TO be hones, is case of annotated tags
>> there is already option tag.forceSignAnnotated. However, non annotated
>> tags are not forced to be signed.
>>
>> Additionally, the proposed option is symmetric with commit.gpgSign.
> 
> Now I'm even more curious.
> 
> I don't think we have the full picture to understand whether this
> change is needed.  When adding a configuration item, we need to be
> able to explain to users what the configuration item is for, and so
> far the only answer I am hearing is "because we do not want to patch
> our build/release script, though we could in principle".  That doesn't
> sound like a compelling reason.
> 
> On the other hand, perhaps the answer is "our build/release script
> does not have a --sign option for the following reason, and this is a
> better interface for configuring it".
> 
> Or perhaps there is an answer that does not involve the build/release
> script.
> 
> But with no answer at all, it is hard to see why we should move
> forward on this patch.
> 
> To be clear, I am not saying that writing the patch is wasted effort.
> E.g. you can continue to use it internally, and it means that once we
> have a clear reason to add this configuration, the patch is there and
> ready to use to do so.
> 
> Thanks again,
> Jonathan

Re: [RFE] Add minimal universal release management capabilities to GIT

2017-10-27 Thread nicolas . mailhot



- Mail original -
De: "Jacob Keller" 

Hi Jacob,


> I think that this could easily be built by a separate script which provides 
> git release command line and uses tags under the hood in a 
> well formed way.

True, the difficulty is not technical, the whole scheme is basic and KISS.

> I wouldn't say that the method outlined here works for all projects but I do 
> agree it's fairly common and could work for many projects

I would be very surprised if there was a strong technical reason that stopped 
any project from adopting this scheme. Like I already wrote, Linux packaging 
tools work by converting public release naming to this scheme (with some 
additional twists, mostly there to help conversion of terminally broken, 
tooling-hostile and usually legacy projects, not worth the pain to import in 
new tooling).

> I think most large projects already use annotated tags and tho they have 
> their own format it works pretty well. 

Raw tags are useless as release ids for tooling so everyone is forced to invent 
something else as soon as the project complexity passes a threshold (that's the 
point were there is no choice but to redefine tags, not the point were it 
starts being useful). I've already detailed why their laxity makes them useless.

> Showing a tool that could help projects create more standardized release tags 
> would be helpful.
> 
> I think such a tool could already be built, scripted to create annotated tags 
> with a well formed name. I don't think you necessarily
> need to have this in core git, tho I do see that your main goal is to 
> piggyback on git itselfs popularity

I see little hope for such a tool. Reimplementation is too trivial and 
convention drift only starts to be acutely painful past a certain size. At that 
size you're almost certain to have already started using a custom 
implementation, with refactoring costs impeding switching to a generic tool.

Basically, it can only be done with a good probability of success by 
piggybacking on something that already federates a large number of Git users:
– Git itself, which is the correct most productive and least painful place for 
everyone involved
– one of the big Git-based forges, ie GitHub or GitLab. I'd expect it would be 
very tempting for one of those to make something that would effectively be a 
better Git than upstream Git, the usual embrace and extend effect.
– development language ecosystems (Python, Ruby, Go, etc). There are already 
many premises of such work since build automation needs ids that can be 
processed by tools.

The problem with letting forges or language ecosystems sort it is that you'll 
end up with functionally equivalent implementations, but divergent 
implementation details that end up wasting everyone's time. Like, decimal 
separator differences, deb vs rpm, car driving side, we humans managed to 
create the same clusterfuck time and time again. And much swearing every time 
you have a project that requires bridging those divergences.

It would be worth it if the divergence and competition helped new 
ground-breaking schemes to emerge but really, look at it, it's not rocket 
science. Everyone has been using about this scheme for decades with little 
changes. The remaining differences are slowly being eroded by the wish to 
automate everything.

Regards,

-- 
Nicolas Mailhot

Re: [PATCH 1/2] xdiff-interface: export comparing and hashing strings

René Scharfe  writes:

> Am 25.10.2017 um 20:49 schrieb Stefan Beller:
>> +/*
>> + * Compare the strings l1 with l2 which are of size s1 and s2 respectively.
>> + * Returns 1 if the strings are deemed equal, 0 otherwise.
>> + * The `flags` given as XDF_WHITESPACE_FLAGS determine how white spaces
>> + * are treated for the comparision.
>> + */
>> +extern int xdiff_compare_lines(const char *l1, long s1,
>> +   const char *l2, long s2, long flags);
>
> With the added comment it's OK here.
>
> Still, I find the tendency in libxdiff to use the shortest possible
> variable names to be hard on the eyes.  That math-like notation may have
> its place in that external library, but I think we should be careful
> lest it spreads.

Yeah, I tend to agree.  The xdiff-interface is at the (surprise!)
interface layer, so we could make it follow the style of either one,
but we seem to have picked up the convention of the lower layer,
so...

By the way, I was looking at the code around this area while
reviewing the cr-at-eol thing, and noticed a couple of things:

 * combine-diff.c special cases only IGNORE_WHITESPACE and
   IGNORE_WHITESPACE_CHANGE but no other IGNORE_WHITESPACE things; I
   have a suspicion that this is not because IGNORE_WHITESPACE_AT_EOL
   does not have to special cased by the codepath, but only because
   the combine-diff.c refactoring predates the introduction of
   ws-at-eol thing?

   The machinery uses its own match_string_spaces() helper; it may
   make sense to use the same xdiff_compare_lines() in its callers
   and get rid of this helper function.

 * diff_setup_done() sets DIFF_FROM_CONTENTS when any of the
   IGNORE_WHITESPACE bits is true, to allow "git diff -q
   --ignore-something" would do the right thing.  We do not however
   give a similar special case to XDF_IGNORE_BLANK_LINES.

   $ echo >>Makefile && git diff $option --ignore-blank-lines Makefile

   exits with 1 when option=--exit-code and it exits with 0 when
   option=-q

For now I'll leve these as #leftoverbits, but I may come back to the
latter soonish.  I won't come back to the former until Stefan's
refactor hits 'master', though.

Re: [PATCH v3] blame: prevent error if range ends past end of file

2017-10-27 Thread Eric Sunshine

On Fri, Oct 27, 2017 at 2:18 AM, Isabella Stephens
 wrote:
> On 27/10/17 12:58 pm, Junio C Hamano wrote:
>> There should be an "is the range sensible?" check after all the
>> tweaking to bottom and top are done, I think.
>
> My mistake. I missed that case. I think this section of code is a little
> hard to read because it avoids treating an empty file as a special case.
> Why not do something like this:
>
>   for (range_i = 0; range_i < range_list.nr; ++range_i) {
>   long bottom, top;
>   if (!lno)
>   die(_("file is empty"));

No need for this conditional to reside within the loop. Hoisting it
outside the loop would (IMO) make the intent even clearer:

if (range_list.nr && !lno)
die(_("file is empty; -L has no effect"));
for (...) {
...

On the other hand, one could argue that "-L," (where comma is the sole
argument to -L), which specifies the entire file, should be allowed
even on an empty file without complaining that the file is empty.
Although it may not seem sensible for a human to specify "-L," perhaps
a tool would do so to override an earlier more restrictive -L range.
However, that may be too esoteric a case to worry about.

>   if (parse_range_arg(range_list.items[range_i].string,
>   nth_line_cb, , lno, anchor,
>   , , sb.path))
>   usage(blame_usage);
>   if (bottom < 1)
>   bottom = 1;
>   if (lno < top)
>   top = lno;
>   if (top < 0 || lno < bottom)
>die(Q_("file %s has only %lu line",
>  "file %s has only %lu lines",
>  lno), path, lno);
>   bottom--;
>   range_set_append_unsafe(, bottom, top);
>   anchor = top + 1;

Re: [PATCH v3] blame: prevent error if range ends past end of file

2017-10-27 Thread Isabella Stephens

On 27/10/17 12:58 pm, Junio C Hamano wrote:
> Isabella Stephens  writes:
> 
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> index 67adaef4d..b5b9db147 100644
>> --- a/builtin/blame.c
>> +++ b/builtin/blame.c
>> @@ -878,13 +878,13 @@ int cmd_blame(int argc, const char **argv, const char 
>> *prefix)
>>  nth_line_cb, , lno, anchor,
>>  , , sb.path))
>>  usage(blame_usage);
>> -if (lno < top || ((lno || bottom) && lno < bottom))
>> +if ((lno || bottom) && lno < bottom)
>>  die(Q_("file %s has only %lu line",
>> "file %s has only %lu lines",
>> lno), path, lno);
>>  if (bottom < 1)
>>  bottom = 1;
>> -if (top < 1)
>> +if (top < 1 || lno < top)
>>  top = lno;
> 
> This section sanity-checks first and then tweaks the values it
> allowed to pass the check.  Because it wants to later fix up an
> overly large "top" by capping to "lno" (i.e. total line number), the
> patch needs to loosen the early sanity-check.  And the "fixed up"
> values are never checked if they are sane.
> 
> For example, with an empty file (i.e. lno == 0), you can ask "git
> blame -L1,-4 ("i.e. "at most four lines, ending at line #1") and the
> code silently accepts the input without noticing that the request is
> an utter nonsense; "file X has only 0 lines" error is given a chance
> to kick in.
> 
> There should be an "is the range sensible?" check after all the
> tweaking to bottom and top are done, I think.

My mistake. I missed that case. I think this section of code is a little
hard to read because it avoids treating an empty file as a special case. 
Why not do something like this:

  for (range_i = 0; range_i < range_list.nr; ++range_i) {
  long bottom, top;
  if (!lno)
  die(_("file is empty"));
  if (parse_range_arg(range_list.items[range_i].string,
  nth_line_cb, , lno, anchor,
  , , sb.path))
  usage(blame_usage);
  if (bottom < 1)
  bottom = 1;
  if (lno < top)
  top = lno;
  if (top < 0 || lno < bottom)
   die(Q_("file %s has only %lu line",
 "file %s has only %lu lines",
 lno), path, lno);
  bottom--;
  range_set_append_unsafe(, bottom, top);
  anchor = top + 1;

We'd also need to change parse_range_arg to always make bottom less than top:

-   if (*begin && *end && *end < *begin) {
+   if (*end < *begin) {
SWAP(*end, *begin);
}

This also fixes the case where the given range is n,-(n+1) (e.g. -L1,-2). At
the moment that will blame from n to the end of the file. My suggested change
would instead blame the first n lines, which makes a lot more sense IMO.

Happy to leave as is if you aren't happy with this suggestion, however.

Re* Consequences of CRLF in index?