Re: git submodules implementation question

[Junio C Hamano]

Junio C Hamano  writes:

> I was wondering if we should unconditionally stuff GIT_DIR= repository location for the submodule> in the cp.env_array passed to
> the function prepare_submodule_repo_env().  As cp.dir will be set to
> the submodule's working tree, there is no need to set GIT_WORK_TREE
> and export it, I think, although it would not hurt.

After checking all callers of prepare_submodule_repo_env() and saw
that cp.dir is set to the top of working directory for the
submodule, I wonder if something as simple and stupid like the
attached patch is sufficient.  Our subprocess will go there, and
there should be a .git embedded directory or a .git file that points
into .git/modules/* in the superproject, and either way, it should
use .git directly underneath it.

 submodule.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/submodule.c b/submodule.c
index 1b5cdfb..e8258f0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1160,4 +1160,5 @@ void prepare_submodule_repo_env(struct argv_array *out)
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
}
+   argv_array_push(out, "GIT_DIR=.git");
 }

Re: Literate programming with git

[Scott Abbey]

> How does the linearify/dendrify work with already non-linear history?

The current implementation using magic strings in commit messages
seems incompatible with existing projects.

How about annotating an existing project with git-notes instead? You
could add notes to existing commits without needing to rewrite history.

Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Sverre Rabbelier]

On Wed, Aug 31, 2016 at 3:36 AM Johannes Schindelin
 wrote:
> On Tue, 30 Aug 2016, Junio C Hamano wrote:
> > Jeff King  writes:
> > > Hmm, interesting. Your approach seems reasonable, but I have to wonder
> > > if writing the pid in the first place is sane.
> > >
> > > I started to write up my reasoning in this email, but realized it was
> > > rapidly becoming the content of a commit message. So here is that
> > > commit.
> >
> > Sounds sensible; if this makes Dscho's "which ones failed in the
> > previous run" simpler, that is even better ;-)
>
> I did not have the time to dig further before now. There must have been a
> good reason why we append the PID.
>
> Sverre, you added that code in 2d84e9f (Modify test-lib.sh to output stats
> to t/test-results/*, 2008-06-08): any idea why the - suffix was
> needed?

I can't really recall, but I think it may have been related to me
doing something like this:
1. Make a change, and start running tests (this takes a long time)
2. Notice a failure, start fixing it, leave tests running to find
further failures
3. Finish fix, first tests are still running, start another run in a
new terminal (possibly of just the one failed test I was fixing) to
see if the fix worked.

Without the pid, the second run would clobber the results from the first run.


If only past-me was more rigorous about writing good commit messages :P.

Re: [PATCH v12 0/8] submodule inline diff format

[Junio C Hamano]

Jacob Keller  writes:

> @@ -523,8 +523,10 @@ char *git_pathdup_submodule(const char *path, const char 
> *fmt, ...)
>   va_start(args, fmt);
>   err = do_submodule_path(, path, fmt, args);
>   va_end(args);
> - if (err)
> + if (err) {
> + strbuf_release();
>   return NULL;
> + }
>   return strbuf_detach(, NULL);
>  }

Thanks.  My copy was lacking this hunk.  Will replace.

Re: git submodules implementation question

[Uma Srinivasan]

Here's one more attempt where I changed prepare_submodule_repo_env()
to take the submodule git dir as an argument.
Sorry for including this long code diff inline. If there's a better
way to have this discussion with code please let me know.

Thanks,
Uma

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9d79f19..0741f6c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -465,7 +465,7 @@ static int clone_submodule(const char *path, const
char *gitdir, const char *url
argv_array_push(, path);

cp.git_cmd = 1;
-   prepare_submodule_repo_env(_array);
+   prepare_submodule_repo_env(_array, gitdir);
cp.no_stdin = 1;

return run_command();
diff --git a/submodule.c b/submodule.c
index 5a62aa2..3d9174a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -522,11 +522,31 @@ static int has_remote(const char *refname, const
struct object_id *oid,
return 1;
 }

+static const char *submodule_git_dir = NULL;
+const char *get_submodule_git_dir(const char *path, struct strbuf *bufptr)
+{
+   strbuf_addf(bufptr, "%s/.git", path);
+   submodule_git_dir = read_gitfile(bufptr->buf);
+   if (!submodule_git_dir)
+   submodule_git_dir = bufptr->buf;
+   if (!is_directory(submodule_git_dir)) {
+   return NULL;
+   }
+   return submodule_git_dir;
+}
+
 static int submodule_needs_pushing(const char *path, const unsigned
char sha1[20])
 {
if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
return 0;

+   struct strbuf gitdirbuf = STRBUF_INIT;
+const char *sm_gitdir = get_submodule_git_dir(path, );
+   if (sm_gitdir == NULL) {
+   strbuf_release();
+   return 0;
+   }
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
struct child_process cp = CHILD_PROCESS_INIT;
const char *argv[] = {"rev-list", NULL, "--not",
"--remotes", "-n", "1" , NULL};
@@ -535,7 +555,7 @@ static int submodule_needs_pushing(const char
*path, const unsigned char sha1[20

argv[1] = sha1_to_hex(sha1);
cp.argv = argv;
-   prepare_submodule_repo_env(_array);
+   prepare_submodule_repo_env(_array, sm_gitdir);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
@@ -551,6 +571,7 @@ static int submodule_needs_pushing(const char
*path, const unsigned char sha1[20
return needs_pushing;
}

+   strbuf_release();
return 0;
 }

@@ -617,12 +638,18 @@ static int push_submodule(const char *path)
if (add_submodule_odb(path))
return 1;

+   struct strbuf gitdirbuf = STRBUF_INIT;
+const char *sm_gitdir = get_submodule_git_dir(path, );
+   if (sm_gitdir == NULL) {
+   strbuf_release();
+   return 0;
+   }
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
struct child_process cp = CHILD_PROCESS_INIT;
const char *argv[] = {"push", NULL};

cp.argv = argv;
-   prepare_submodule_repo_env(_array);
+   prepare_submodule_repo_env(_array, sm_gitdir);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.dir = path;
@@ -631,6 +658,7 @@ static int push_submodule(const char *path)
close(cp.out);
}

+   strbuf_release();
return 1;
 }

@@ -665,10 +693,16 @@ static int is_submodule_commit_present(const
char *path, unsigned char sha1[20])
struct child_process cp = CHILD_PROCESS_INIT;
const char *argv[] = {"rev-list", "-n", "1", NULL,
"--not", "--all", NULL};
struct strbuf buf = STRBUF_INIT;
+   struct strbuf gitdirbuf = STRBUF_INIT;
+   const char *sm_gitdir = get_submodule_git_dir(path, );
+   if (sm_gitdir == NULL) {
+   strbuf_release();
+   return 0;
+   }

argv[3] = sha1_to_hex(sha1);
cp.argv = argv;
-   prepare_submodule_repo_env(_array);
+   prepare_submodule_repo_env(_array, sm_gitdir);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.dir = path;
@@ -676,6 +710,7 @@ static int is_submodule_commit_present(const char
*path, unsigned char sha1[20])
is_present = 1;

strbuf_release();
+   strbuf_release();
}
return is_present;
 }
@@ -851,7 +886,7 @@ static int get_next_submodule(struct child_process *cp,
if (is_directory(git_dir)) {
child_process_init(cp);
cp->dir = strbuf_detach(_path, NULL);
-   prepare_submodule_repo_env(>env_array);
+   

Re: [GIT PULL] l10n updates for 2.10.0 round 2

[Trần Ngọc Quân]

On 31/08/2016 21:14, Jiang Xin wrote:
> Hi Junio,
> 
> Would you please pull the following git l10n updates.
Please wait! Jiang Xin probably missing pull my one commit[1].

[1]



Thanks,

-- 
Trần Ngọc Quân.

Re: [PATCH v12 0/8] submodule inline diff format

[Stefan Beller]

On Wed, Aug 31, 2016 at 4:27 PM, Jacob Keller  wrote:
> From: Jacob Keller 
>
> Hopefully the final revision here. I've squashed in the memory leak fix
> suggested by Stefan, and the suggested changes from Junio, including his
> re-worded commit messages.
>
> interdiff between v11 and v12
> diff --git c/path.c w/path.c
> index 3dbc4478a4aa..ba60c9849ef7 100644
> --- c/path.c
> +++ w/path.c
> @@ -467,7 +467,7 @@ const char *worktree_git_path(const struct worktree *wt, 
> const char *fmt, ...)
> return pathname->buf;
>  }
>
> -/* Returns 0 on success, non-zero on failure. */
> +/* Returns 0 on success, negative on failure. */
>  #define SUBMODULE_PATH_ERR_NOT_CONFIGURED -1
>  static int do_submodule_path(struct strbuf *buf, const char *path,
>  const char *fmt, va_list args)
> @@ -523,8 +523,10 @@ char *git_pathdup_submodule(const char *path, const char 
> *fmt, ...)
> va_start(args, fmt);
> err = do_submodule_path(, path, fmt, args);
> va_end(args);
> -   if (err)
> +   if (err) {
> +   strbuf_release();
> return NULL;
> +   }
> return strbuf_detach(, NULL);
>  }
>

Skimmed all patches very quickly and they look good to me.

Thanks,
Stefan

[PATCH v12 0/8] submodule inline diff format

[Jacob Keller]

From: Jacob Keller 

Hopefully the final revision here. I've squashed in the memory leak fix
suggested by Stefan, and the suggested changes from Junio, including his
re-worded commit messages.

interdiff between v11 and v12
diff --git c/path.c w/path.c
index 3dbc4478a4aa..ba60c9849ef7 100644
--- c/path.c
+++ w/path.c
@@ -467,7 +467,7 @@ const char *worktree_git_path(const struct worktree *wt, 
const char *fmt, ...)
return pathname->buf;
 }
 
-/* Returns 0 on success, non-zero on failure. */
+/* Returns 0 on success, negative on failure. */
 #define SUBMODULE_PATH_ERR_NOT_CONFIGURED -1
 static int do_submodule_path(struct strbuf *buf, const char *path,
 const char *fmt, va_list args)
@@ -523,8 +523,10 @@ char *git_pathdup_submodule(const char *path, const char 
*fmt, ...)
va_start(args, fmt);
err = do_submodule_path(, path, fmt, args);
va_end(args);
-   if (err)
+   if (err) {
+   strbuf_release();
return NULL;
+   }
return strbuf_detach(, NULL);
 }
 
>8

Jacob Keller (7):
  cache: add empty_tree_oid object and helper function
  graph: add support for --line-prefix on all graph-aware output
  diff: prepare for additional submodule formats
  allow do_submodule_path to work even if submodule isn't checked out
  submodule: convert show_submodule_summary to use struct object_id *
  submodule: refactor show_submodule_summary with helper function
  diff: teach diff to display submodule difference with an inline diff

Junio C Hamano (1):
  diff.c: remove output_prefix_length field

 Documentation/diff-config.txt  |   9 +-
 Documentation/diff-options.txt |  20 +-
 builtin/rev-list.c |  70 +-
 cache.h|  29 +-
 diff.c |  64 +-
 diff.h |  11 +-
 graph.c| 100 ++-
 graph.h|  22 +-
 log-tree.c |   5 +-
 path.c |  39 +-
 refs/files-backend.c   |   8 +-
 sha1_file.c|   6 +
 submodule.c| 190 +-
 submodule.h|   8 +-
 t/t4013-diff-various.sh|   6 +
 ...diff.diff_--line-prefix=abc_master_master^_side |  29 +
 t/t4013/diff.diff_--line-prefix_--cached_--_file0  |  15 +
 t/t4059-diff-submodule-not-initialized.sh  | 127 
 t/t4060-diff-submodule-option-diff-format.sh   | 749 +
 t/t4202-log.sh | 323 +
 20 files changed, 1666 insertions(+), 164 deletions(-)
 create mode 100644 t/t4013/diff.diff_--line-prefix=abc_master_master^_side
 create mode 100644 t/t4013/diff.diff_--line-prefix_--cached_--_file0
 create mode 100755 t/t4059-diff-submodule-not-initialized.sh
 create mode 100755 t/t4060-diff-submodule-option-diff-format.sh

-- 
2.10.0.rc2.311.g2bd286e

[PATCH v12 5/8] allow do_submodule_path to work even if submodule isn't checked out

[Jacob Keller]

From: Jacob Keller 

Currently, do_submodule_path will attempt locating the .git directory by
using read_gitfile on /.git. If this fails it just assumes the
/.git is actually a git directory.

This is good because it allows for handling submodules which were cloned
in a regular manner first before being added to the superproject.

Unfortunately this fails if the  is not actually checked out any
longer, such as by removing the directory.

Fix this by checking if the directory we found is actually a gitdir. In
the case it is not, attempt to lookup the submodule configuration and
find the name of where it is stored in the .git/modules/ directory of
the superproject.

If we can't locate the submodule configuration, this might occur because
for example a submodule gitlink was added but the corresponding
.gitmodules file was not properly updated.  A die() here would not be
pleasant to the users of submodule diff formats, so instead, modify
do_submodule_path() to return an error code:

 - git_pathdup_submodule() returns NULL when we fail to find a path.
 - strbuf_git_path_submodule() propagates the error code to the caller.

Modify the callers of these functions to check the error code and fail
properly. This ensures we don't attempt to use a bad path that doesn't
match the corresponding submodule.

Because this change fixes add_submodule_odb() to work even if the
submodule is not checked out, update the wording of the submodule log
diff format to correctly display that the submodule is "not initialized"
instead of "not checked out"

Add tests to ensure this change works as expected.

Signed-off-by: Jacob Keller 
Signed-off-by: Junio C Hamano 
---
 cache.h   |   4 +-
 path.c|  39 +++--
 refs/files-backend.c  |   8 +-
 submodule.c   |   6 +-
 t/t4059-diff-submodule-not-initialized.sh | 127 ++
 5 files changed, 173 insertions(+), 11 deletions(-)
 create mode 100755 t/t4059-diff-submodule-not-initialized.sh

diff --git a/cache.h b/cache.h
index 2ac37ee070c0..4d62df5cce1c 100644
--- a/cache.h
+++ b/cache.h
@@ -819,8 +819,8 @@ extern void strbuf_git_common_path(struct strbuf *sb, const 
char *fmt, ...)
__attribute__((format (printf, 2, 3)));
 extern char *git_path_buf(struct strbuf *buf, const char *fmt, ...)
__attribute__((format (printf, 2, 3)));
-extern void strbuf_git_path_submodule(struct strbuf *sb, const char *path,
- const char *fmt, ...)
+extern int strbuf_git_path_submodule(struct strbuf *sb, const char *path,
+const char *fmt, ...)
__attribute__((format (printf, 3, 4)));
 extern char *git_pathdup(const char *fmt, ...)
__attribute__((format (printf, 1, 2)));
diff --git a/path.c b/path.c
index 17551c483476..ba60c9849ef7 100644
--- a/path.c
+++ b/path.c
@@ -6,6 +6,7 @@
 #include "string-list.h"
 #include "dir.h"
 #include "worktree.h"
+#include "submodule-config.h"
 
 static int get_st_mode_bits(const char *path, int *mode)
 {
@@ -466,12 +467,16 @@ const char *worktree_git_path(const struct worktree *wt, 
const char *fmt, ...)
return pathname->buf;
 }
 
-static void do_submodule_path(struct strbuf *buf, const char *path,
- const char *fmt, va_list args)
+/* Returns 0 on success, negative on failure. */
+#define SUBMODULE_PATH_ERR_NOT_CONFIGURED -1
+static int do_submodule_path(struct strbuf *buf, const char *path,
+const char *fmt, va_list args)
 {
const char *git_dir;
struct strbuf git_submodule_common_dir = STRBUF_INIT;
struct strbuf git_submodule_dir = STRBUF_INIT;
+   const struct submodule *sub;
+   int err = 0;
 
strbuf_addstr(buf, path);
strbuf_complete(buf, '/');
@@ -482,6 +487,17 @@ static void do_submodule_path(struct strbuf *buf, const 
char *path,
strbuf_reset(buf);
strbuf_addstr(buf, git_dir);
}
+   if (!is_git_directory(buf->buf)) {
+   gitmodules_config();
+   sub = submodule_from_path(null_sha1, path);
+   if (!sub) {
+   err = SUBMODULE_PATH_ERR_NOT_CONFIGURED;
+   goto cleanup;
+   }
+   strbuf_reset(buf);
+   strbuf_git_path(buf, "%s/%s", "modules", sub->name);
+   }
+
strbuf_addch(buf, '/');
strbuf_addbuf(_submodule_dir, buf);
 
@@ -492,27 +508,38 @@ static void do_submodule_path(struct strbuf *buf, const 
char *path,
 
strbuf_cleanup_path(buf);
 
+cleanup:
strbuf_release(_submodule_dir);
strbuf_release(_submodule_common_dir);
+
+   return err;
 }
 
 char *git_pathdup_submodule(const char *path, const char *fmt, ...)
 {
+   int err;
va_list args;

[PATCH v12 4/8] diff: prepare for additional submodule formats

[Jacob Keller]

From: Jacob Keller 

A future patch will add a new format for displaying the difference of
a submodule. Make it easier by changing how we store the current
selected format. Replace the DIFF_OPT flag with an enumeration, as each
format will be mutually exclusive.

Signed-off-by: Jacob Keller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 12 ++--
 diff.h |  7 ++-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/diff.c b/diff.c
index 1496ff405d0d..1380bbe250ad 100644
--- a/diff.c
+++ b/diff.c
@@ -132,9 +132,9 @@ static int parse_dirstat_params(struct diff_options 
*options, const char *params
 static int parse_submodule_params(struct diff_options *options, const char 
*value)
 {
if (!strcmp(value, "log"))
-   DIFF_OPT_SET(options, SUBMODULE_LOG);
+   options->submodule_format = DIFF_SUBMODULE_LOG;
else if (!strcmp(value, "short"))
-   DIFF_OPT_CLR(options, SUBMODULE_LOG);
+   options->submodule_format = DIFF_SUBMODULE_SHORT;
else
return -1;
return 0;
@@ -2300,9 +2300,9 @@ static void builtin_diff(const char *name_a,
struct strbuf header = STRBUF_INIT;
const char *line_prefix = diff_line_prefix(o);
 
-   if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
-   (!one->mode || S_ISGITLINK(one->mode)) &&
-   (!two->mode || S_ISGITLINK(two->mode))) {
+   if (o->submodule_format == DIFF_SUBMODULE_LOG &&
+   (!one->mode || S_ISGITLINK(one->mode)) &&
+   (!two->mode || S_ISGITLINK(two->mode))) {
const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
show_submodule_summary(o->file, one->path ? one->path : 
two->path,
@@ -3916,7 +3916,7 @@ int diff_opt_parse(struct diff_options *options,
DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);
handle_ignore_submodules_arg(options, arg);
} else if (!strcmp(arg, "--submodule"))
-   DIFF_OPT_SET(options, SUBMODULE_LOG);
+   options->submodule_format = DIFF_SUBMODULE_LOG;
else if (skip_prefix(arg, "--submodule=", ))
return parse_submodule_opt(options, arg);
else if (skip_prefix(arg, "--ws-error-highlight=", ))
diff --git a/diff.h b/diff.h
index 219a28aa0f9c..14be35d0176c 100644
--- a/diff.h
+++ b/diff.h
@@ -83,7 +83,6 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct 
diff_options *opt, void *data)
 #define DIFF_OPT_DIRSTAT_BY_FILE (1 << 20)
 #define DIFF_OPT_ALLOW_TEXTCONV  (1 << 21)
 #define DIFF_OPT_DIFF_FROM_CONTENTS  (1 << 22)
-#define DIFF_OPT_SUBMODULE_LOG   (1 << 23)
 #define DIFF_OPT_DIRTY_SUBMODULES(1 << 24)
 #define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)
 #define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)
@@ -110,6 +109,11 @@ enum diff_words_type {
DIFF_WORDS_COLOR
 };
 
+enum diff_submodule_format {
+   DIFF_SUBMODULE_SHORT = 0,
+   DIFF_SUBMODULE_LOG
+};
+
 struct diff_options {
const char *orderfile;
const char *pickaxe;
@@ -157,6 +161,7 @@ struct diff_options {
int stat_count;
const char *word_regex;
enum diff_words_type word_diff;
+   enum diff_submodule_format submodule_format;
 
/* this is set by diffcore for DIFF_FORMAT_PATCH */
int found_changes;
-- 
2.10.0.rc2.311.g2bd286e

[PATCH v12 2/8] diff.c: remove output_prefix_length field

[Jacob Keller]

From: Junio C Hamano 

"diff/log --stat" has a logic that determines the display columns
available for the diffstat part of the output and apportions it for
pathnames and diffstat graph automatically.

5e71a84a (Add output_prefix_length to diff_options, 2012-04-16)
added the output_prefix_length field to diff_options structure to
allow this logic to subtract the display columns used for the
history graph part from the total "terminal width"; this matters
when the "git log --graph -p" option is in use.

The field must be set to the number of display columns needed to
show the output from the output_prefix() callback, which is error
prone.  As there is only one user of the field, and the user has the
actual value of the prefix string, let's get rid of the field and
have the user count the display width itself.

Signed-off-by: Junio C Hamano 
---
 diff.c  | 2 +-
 diff.h  | 1 -
 graph.c | 2 --
 3 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index b43d3dd2ecb7..ae069c303077 100644
--- a/diff.c
+++ b/diff.c
@@ -1625,7 +1625,7 @@ static void show_stats(struct diffstat_t *data, struct 
diff_options *options)
 */
 
if (options->stat_width == -1)
-   width = term_columns() - options->output_prefix_length;
+   width = term_columns() - strlen(line_prefix);
else
width = options->stat_width ? options->stat_width : 80;
number_width = decimal_width(max_change) > number_width ?
diff --git a/diff.h b/diff.h
index 125447be09eb..49e4aaafb2da 100644
--- a/diff.h
+++ b/diff.h
@@ -174,7 +174,6 @@ struct diff_options {
diff_format_fn_t format_callback;
void *format_callback_data;
diff_prefix_fn_t output_prefix;
-   int output_prefix_length;
void *output_prefix_data;
 
int diff_path_counter;
diff --git a/graph.c b/graph.c
index dd1720148dc5..a46803840511 100644
--- a/graph.c
+++ b/graph.c
@@ -197,7 +197,6 @@ static struct strbuf *diff_output_prefix_callback(struct 
diff_options *opt, void
assert(opt);
assert(graph);
 
-   opt->output_prefix_length = graph->width;
strbuf_reset();
graph_padding_line(graph, );
return 
@@ -245,7 +244,6 @@ struct git_graph *graph_init(struct rev_info *opt)
 */
opt->diffopt.output_prefix = diff_output_prefix_callback;
opt->diffopt.output_prefix_data = graph;
-   opt->diffopt.output_prefix_length = 0;
 
return graph;
 }
-- 
2.10.0.rc2.311.g2bd286e

[PATCH v12 6/8] submodule: convert show_submodule_summary to use struct object_id *

[Jacob Keller]

From: Jacob Keller 

Since we're going to be changing this function in a future patch, lets
go ahead and convert this to use object_id now.

Signed-off-by: Jacob Keller 
Signed-off-by: Junio C Hamano 
---
 diff.c  |  2 +-
 submodule.c | 16 
 submodule.h |  2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/diff.c b/diff.c
index 1380bbe250ad..a74e6e06dfb6 100644
--- a/diff.c
+++ b/diff.c
@@ -2307,7 +2307,7 @@ static void builtin_diff(const char *name_a,
const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
show_submodule_summary(o->file, one->path ? one->path : 
two->path,
line_prefix,
-   one->oid.hash, two->oid.hash,
+   >oid, >oid,
two->dirty_submodule,
meta, del, add, reset);
return;
diff --git a/submodule.c b/submodule.c
index 6096cf428be7..7cb236b0a108 100644
--- a/submodule.c
+++ b/submodule.c
@@ -337,7 +337,7 @@ static void print_submodule_summary(struct rev_info *rev, 
FILE *f,
 
 void show_submodule_summary(FILE *f, const char *path,
const char *line_prefix,
-   unsigned char one[20], unsigned char two[20],
+   struct object_id *one, struct object_id *two,
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset)
 {
@@ -347,14 +347,14 @@ void show_submodule_summary(FILE *f, const char *path,
struct strbuf sb = STRBUF_INIT;
int fast_forward = 0, fast_backward = 0;
 
-   if (is_null_sha1(two))
+   if (is_null_oid(two))
message = "(submodule deleted)";
else if (add_submodule_odb(path))
message = "(not initialized)";
-   else if (is_null_sha1(one))
+   else if (is_null_oid(one))
message = "(new submodule)";
-   else if (!(left = lookup_commit_reference(one)) ||
-!(right = lookup_commit_reference(two)))
+   else if (!(left = lookup_commit_reference(one->hash)) ||
+!(right = lookup_commit_reference(two->hash)))
message = "(commits not present)";
else if (prepare_submodule_summary(, path, left, right,
   _forward, _backward))
@@ -367,16 +367,16 @@ void show_submodule_summary(FILE *f, const char *path,
fprintf(f, "%sSubmodule %s contains modified content\n",
line_prefix, path);
 
-   if (!hashcmp(one, two)) {
+   if (!oidcmp(one, two)) {
strbuf_release();
return;
}
 
strbuf_addf(, "%s%sSubmodule %s %s..", line_prefix, meta, path,
-   find_unique_abbrev(one, DEFAULT_ABBREV));
+   find_unique_abbrev(one->hash, DEFAULT_ABBREV));
if (!fast_backward && !fast_forward)
strbuf_addch(, '.');
-   strbuf_addf(, "%s", find_unique_abbrev(two, DEFAULT_ABBREV));
+   strbuf_addf(, "%s", find_unique_abbrev(two->hash, DEFAULT_ABBREV));
if (message)
strbuf_addf(, " %s%s\n", message, reset);
else
diff --git a/submodule.h b/submodule.h
index 2af939099819..d83df57e24ff 100644
--- a/submodule.h
+++ b/submodule.h
@@ -43,7 +43,7 @@ const char *submodule_strategy_to_string(const struct 
submodule_update_strategy
 void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
 void show_submodule_summary(FILE *f, const char *path,
const char *line_prefix,
-   unsigned char one[20], unsigned char two[20],
+   struct object_id *one, struct object_id *two,
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset);
 void set_config_fetch_recurse_submodules(int value);
-- 
2.10.0.rc2.311.g2bd286e

[PATCH v12 7/8] submodule: refactor show_submodule_summary with helper function

[Jacob Keller]

From: Jacob Keller 

A future patch is going to add a new submodule diff format which
displays an inline diff of the submodule changes. To make this easier,
and to ensure that both submodule diff formats use the same initial
header, factor out show_submodule_header() function which will print the
current submodule header line, and then leave the show_submodule_summary
function to lookup and print the submodule log format.

This does create one format change in that "(revision walker failed)"
will now be displayed on its own line rather than as part of the message
because we no longer perform this step directly in the header display
flow. However, this is a rare case as most causes of the failure will be
due to a missing commit which we already check for and avoid previously.
flow. However, this is a rare case and shouldn't impact much.

Signed-off-by: Jacob Keller 
Signed-off-by: Junio C Hamano 
---
 submodule.c | 115 +++-
 1 file changed, 82 insertions(+), 33 deletions(-)

diff --git a/submodule.c b/submodule.c
index 7cb236b0a108..2d88c555895d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -280,9 +280,9 @@ void handle_ignore_submodules_arg(struct diff_options 
*diffopt,
 
 static int prepare_submodule_summary(struct rev_info *rev, const char *path,
struct commit *left, struct commit *right,
-   int *fast_forward, int *fast_backward)
+   struct commit_list *merge_bases)
 {
-   struct commit_list *merge_bases, *list;
+   struct commit_list *list;
 
init_revisions(rev, NULL);
setup_revisions(0, NULL, rev, NULL);
@@ -291,13 +291,6 @@ static int prepare_submodule_summary(struct rev_info *rev, 
const char *path,
left->object.flags |= SYMMETRIC_LEFT;
add_pending_object(rev, >object, path);
add_pending_object(rev, >object, path);
-   merge_bases = get_merge_bases(left, right);
-   if (merge_bases) {
-   if (merge_bases->item == left)
-   *fast_forward = 1;
-   else if (merge_bases->item == right)
-   *fast_backward = 1;
-   }
for (list = merge_bases; list; list = list->next) {
list->item->object.flags |= UNINTERESTING;
add_pending_object(rev, >item->object,
@@ -335,31 +328,23 @@ static void print_submodule_summary(struct rev_info *rev, 
FILE *f,
strbuf_release();
 }
 
-void show_submodule_summary(FILE *f, const char *path,
+/* Helper function to display the submodule header line prior to the full
+ * summary output. If it can locate the submodule objects directory it will
+ * attempt to lookup both the left and right commits and put them into the
+ * left and right pointers.
+ */
+static void show_submodule_header(FILE *f, const char *path,
const char *line_prefix,
struct object_id *one, struct object_id *two,
unsigned dirty_submodule, const char *meta,
-   const char *del, const char *add, const char *reset)
+   const char *reset,
+   struct commit **left, struct commit **right,
+   struct commit_list **merge_bases)
 {
-   struct rev_info rev;
-   struct commit *left = NULL, *right = NULL;
const char *message = NULL;
struct strbuf sb = STRBUF_INIT;
int fast_forward = 0, fast_backward = 0;
 
-   if (is_null_oid(two))
-   message = "(submodule deleted)";
-   else if (add_submodule_odb(path))
-   message = "(not initialized)";
-   else if (is_null_oid(one))
-   message = "(new submodule)";
-   else if (!(left = lookup_commit_reference(one->hash)) ||
-!(right = lookup_commit_reference(two->hash)))
-   message = "(commits not present)";
-   else if (prepare_submodule_summary(, path, left, right,
-  _forward, _backward))
-   message = "(revision walker failed)";
-
if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
fprintf(f, "%sSubmodule %s contains untracked content\n",
line_prefix, path);
@@ -367,11 +352,46 @@ void show_submodule_summary(FILE *f, const char *path,
fprintf(f, "%sSubmodule %s contains modified content\n",
line_prefix, path);
 
+   if (is_null_oid(one))
+   message = "(new submodule)";
+   else if (is_null_oid(two))
+   message = "(submodule deleted)";
+
+   if (add_submodule_odb(path)) {
+   if (!message)
+   message = "(not initialized)";
+   goto output_header;
+   }
+
+   /*
+* Attempt to lookup the commit references, and determine if this is
+* a fast forward or fast backwards update.
+*/
+   *left = 

[PATCH v12 3/8] graph: add support for --line-prefix on all graph-aware output

[Jacob Keller]

From: Jacob Keller 

Add an extension to git-diff and git-log (and any other graph-aware
displayable output) such that "--line-prefix=" will print the
additional line-prefix on every line of output.

To make this work, we have to fix a few bugs in the graph API that force
graph_show_commit_msg to be used only when you have a valid graph.
Additionally, we extend the default_diff_output_prefix handler to work
even when no graph is enabled.

This is somewhat of a hack on top of the graph API, but I think it
should be acceptable here.

This will be used by a future extension of submodule display which
displays the submodule diff as the actual diff between the pre and post
commit in the submodule project.

Add some tests for both git-log and git-diff to ensure that the prefix
is honored correctly.

Signed-off-by: Jacob Keller 
Signed-off-by: Junio C Hamano 
---
 Documentation/diff-options.txt |   3 +
 builtin/rev-list.c |  70 ++---
 diff.c |   7 +
 diff.h |   2 +
 graph.c|  98 ---
 graph.h|  22 +-
 log-tree.c |   5 +-
 t/t4013-diff-various.sh|   6 +
 ...diff.diff_--line-prefix=abc_master_master^_side |  29 ++
 t/t4013/diff.diff_--line-prefix_--cached_--_file0  |  15 +
 t/t4202-log.sh | 323 +
 11 files changed, 502 insertions(+), 78 deletions(-)
 create mode 100644 t/t4013/diff.diff_--line-prefix=abc_master_master^_side
 create mode 100644 t/t4013/diff.diff_--line-prefix_--cached_--_file0

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 705a87394200..cc4342e2034d 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -569,5 +569,8 @@ endif::git-format-patch[]
 --no-prefix::
Do not show any source or destination prefix.
 
+--line-prefix=::
+   Prepend an additional prefix to every line of output.
+
 For more detailed explanation on these common options, see also
 linkgit:gitdiffcore[7].
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0ba82b1635b6..8479f6ed28aa 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -122,48 +122,40 @@ static void show_commit(struct commit *commit, void *data)
ctx.fmt = revs->commit_format;
ctx.output_encoding = get_log_output_encoding();
pretty_print_commit(, commit, );
-   if (revs->graph) {
-   if (buf.len) {
-   if (revs->commit_format != CMIT_FMT_ONELINE)
-   graph_show_oneline(revs->graph);
+   if (buf.len) {
+   if (revs->commit_format != CMIT_FMT_ONELINE)
+   graph_show_oneline(revs->graph);
 
-   graph_show_commit_msg(revs->graph, );
+   graph_show_commit_msg(revs->graph, stdout, );
 
-   /*
-* Add a newline after the commit message.
-*
-* Usually, this newline produces a blank
-* padding line between entries, in which case
-* we need to add graph padding on this line.
-*
-* However, the commit message may not end in a
-* newline.  In this case the newline simply
-* ends the last line of the commit message,
-* and we don't need any graph output.  (This
-* always happens with CMIT_FMT_ONELINE, and it
-* happens with CMIT_FMT_USERFORMAT when the
-* format doesn't explicitly end in a newline.)
-*/
-   if (buf.len && buf.buf[buf.len - 1] == '\n')
-   graph_show_padding(revs->graph);
-   putchar('\n');
-   } else {
-   /*
-* If the message buffer is empty, just show
-* the rest of the graph output for this
-* commit.
-*/
-   if (graph_show_remainder(revs->graph))
-   putchar('\n');
-   if (revs->commit_format == CMIT_FMT_ONELINE)
-   putchar('\n');
-  

[PATCH v12 8/8] diff: teach diff to display submodule difference with an inline diff

[Jacob Keller]

From: Jacob Keller 

Teach git-diff and friends a new format for displaying the difference
of a submodule. The new format is an inline diff of the contents of the
submodule between the commit range of the update. This allows the user
to see the actual code change caused by a submodule update.

Add tests for the new format and option.

Signed-off-by: Jacob Keller 
Signed-off-by: Junio C Hamano 
---
 Documentation/diff-config.txt|   9 +-
 Documentation/diff-options.txt   |  17 +-
 diff.c   |  31 +-
 diff.h   |   3 +-
 submodule.c  |  69 +++
 submodule.h  |   6 +
 t/t4060-diff-submodule-option-diff-format.sh | 749 +++
 7 files changed, 863 insertions(+), 21 deletions(-)
 create mode 100755 t/t4060-diff-submodule-option-diff-format.sh

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index d5a5b17d5088..0eded24034b5 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -122,10 +122,11 @@ diff.suppressBlankEmpty::
 
 diff.submodule::
Specify the format in which differences in submodules are
-   shown.  The "log" format lists the commits in the range like
-   linkgit:git-submodule[1] `summary` does.  The "short" format
-   format just shows the names of the commits at the beginning
-   and end of the range.  Defaults to short.
+   shown.  The "short" format just shows the names of the commits
+   at the beginning and end of the range. The "log" format lists
+   the commits in the range like linkgit:git-submodule[1] `summary`
+   does. The "diff" format shows an inline diff of the changed
+   contents of the submodule. Defaults to "short".
 
 diff.wordRegex::
A POSIX Extended Regular Expression used to determine what is a "word"
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index cc4342e2034d..7805a0ccadf2 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -210,13 +210,16 @@ any of those replacements occurred.
of the `--diff-filter` option on what the status letters mean.
 
 --submodule[=]::
-   Specify how differences in submodules are shown.  When `--submodule`
-   or `--submodule=log` is given, the 'log' format is used.  This format 
lists
-   the commits in the range like linkgit:git-submodule[1] `summary` does.
-   Omitting the `--submodule` option or specifying `--submodule=short`,
-   uses the 'short' format. This format just shows the names of the commits
-   at the beginning and end of the range.  Can be tweaked via the
-   `diff.submodule` configuration variable.
+   Specify how differences in submodules are shown.  When specifying
+   `--submodule=short` the 'short' format is used.  This format just
+   shows the names of the commits at the beginning and end of the range.
+   When `--submodule` or `--submodule=log` is specified, the 'log'
+   format is used.  This format lists the commits in the range like
+   linkgit:git-submodule[1] `summary` does.  When `--submodule=diff`
+   is specified, the 'diff' format is used.  This format shows an
+   inline diff of the changes in the submodule contents between the
+   commit range.  Defaults to `diff.submodule` or the 'short' format
+   if the config option is unset.
 
 --color[=]::
Show colored diff.
diff --git a/diff.c b/diff.c
index a74e6e06dfb6..b002fdfb8180 100644
--- a/diff.c
+++ b/diff.c
@@ -135,6 +135,8 @@ static int parse_submodule_params(struct diff_options 
*options, const char *valu
options->submodule_format = DIFF_SUBMODULE_LOG;
else if (!strcmp(value, "short"))
options->submodule_format = DIFF_SUBMODULE_SHORT;
+   else if (!strcmp(value, "diff"))
+   options->submodule_format = DIFF_SUBMODULE_INLINE_DIFF;
else
return -1;
return 0;
@@ -2300,6 +2302,15 @@ static void builtin_diff(const char *name_a,
struct strbuf header = STRBUF_INIT;
const char *line_prefix = diff_line_prefix(o);
 
+   diff_set_mnemonic_prefix(o, "a/", "b/");
+   if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
+   a_prefix = o->b_prefix;
+   b_prefix = o->a_prefix;
+   } else {
+   a_prefix = o->a_prefix;
+   b_prefix = o->b_prefix;
+   }
+
if (o->submodule_format == DIFF_SUBMODULE_LOG &&
(!one->mode || S_ISGITLINK(one->mode)) &&
(!two->mode || S_ISGITLINK(two->mode))) {
@@ -2311,6 +2322,17 @@ static void builtin_diff(const char *name_a,
two->dirty_submodule,
meta, del, add, reset);
return;
+   } else if 

[PATCH v12 1/8] cache: add empty_tree_oid object and helper function

[Jacob Keller]

From: Jacob Keller 

Similar to is_null_oid(), and is_empty_blob_sha1() add an
empty_tree_oid along with helper function is_empty_tree_oid(). For
completeness, also add an "is_empty_tree_sha1()",
"is_empty_blob_sha1()", "is_empty_tree_oid()" and "is_empty_blob_oid()"
helpers.

To ensure we only get one singleton, implement EMPTY_BLOB_SHA1_BIN as
simply getting the hash of empty_blob_oid structure.

Signed-off-by: Jacob Keller 
Signed-off-by: Junio C Hamano 
---
 cache.h | 25 +
 sha1_file.c |  6 ++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 95a0bd397a98..2ac37ee070c0 100644
--- a/cache.h
+++ b/cache.h
@@ -953,22 +953,39 @@ static inline void oidclr(struct object_id *oid)
 #define EMPTY_TREE_SHA1_BIN_LITERAL \
 "\x4b\x82\x5d\xc6\x42\xcb\x6e\xb9\xa0\x60" \
 "\xe5\x4b\xf8\xd6\x92\x88\xfb\xee\x49\x04"
-#define EMPTY_TREE_SHA1_BIN \
-((const unsigned char *) EMPTY_TREE_SHA1_BIN_LITERAL)
+extern const struct object_id empty_tree_oid;
+#define EMPTY_TREE_SHA1_BIN (empty_tree_oid.hash)
 
 #define EMPTY_BLOB_SHA1_HEX \
"e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"
 #define EMPTY_BLOB_SHA1_BIN_LITERAL \
"\xe6\x9d\xe2\x9b\xb2\xd1\xd6\x43\x4b\x8b" \
"\x29\xae\x77\x5a\xd8\xc2\xe4\x8c\x53\x91"
-#define EMPTY_BLOB_SHA1_BIN \
-   ((const unsigned char *) EMPTY_BLOB_SHA1_BIN_LITERAL)
+extern const struct object_id empty_blob_oid;
+#define EMPTY_BLOB_SHA1_BIN (empty_blob_oid.hash)
+
 
 static inline int is_empty_blob_sha1(const unsigned char *sha1)
 {
return !hashcmp(sha1, EMPTY_BLOB_SHA1_BIN);
 }
 
+static inline int is_empty_blob_oid(const struct object_id *oid)
+{
+   return !hashcmp(oid->hash, EMPTY_BLOB_SHA1_BIN);
+}
+
+static inline int is_empty_tree_sha1(const unsigned char *sha1)
+{
+   return !hashcmp(sha1, EMPTY_TREE_SHA1_BIN);
+}
+
+static inline int is_empty_tree_oid(const struct object_id *oid)
+{
+   return !hashcmp(oid->hash, EMPTY_TREE_SHA1_BIN);
+}
+
+
 int git_mkstemp(char *path, size_t n, const char *template);
 
 /* set default permissions by passing mode arguments to open(2) */
diff --git a/sha1_file.c b/sha1_file.c
index 02940f192030..5b8553d69e06 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -38,6 +38,12 @@ static inline uintmax_t sz_fmt(size_t s) { return s; }
 
 const unsigned char null_sha1[20];
 const struct object_id null_oid;
+const struct object_id empty_tree_oid = {
+   EMPTY_TREE_SHA1_BIN_LITERAL
+};
+const struct object_id empty_blob_oid = {
+   EMPTY_BLOB_SHA1_BIN_LITERAL
+};
 
 /*
  * This is meant to hold a *small* number of objects that you would
-- 
2.10.0.rc2.311.g2bd286e

Re: [PATCH 09/22] sequencer: completely revamp the "todo" script parsing

[Stefan Beller]

On Wed, Aug 31, 2016 at 10:29 AM, Jakub Narębski  wrote:

>
> BTW. perhaps we would be able to continue with 'git continue', regardless
> of what we have started with, I wonder...
>

git continue as a shorthand for `git  --continue` sounds great.

If we were to introduce that, I think we would need to unify the rest
as well then, e.g.
Both revert as well as cherry-pick have --quit as well as --abort,
but rebase doesn't have --quit documented, but instead an additional
--skip  and --edit-todo

Would we pull these all up as a top level command? (That sounds not so
great to me)

Re: [PATCH v11 0/8] submodule inline diff format

[Jacob Keller]

On Wed, Aug 31, 2016 at 9:45 AM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>>> We probably should release for the error case. I'll do that. I don't
>>> believe do_submodule_path ensures that the passed in argument is
>>> guaranteed to not be initialized or used.
>>>
>>> Thanks,
>>> Jake
>>
>> Here's the squash for this fix.
>
> It seems that the topic has been quiet since the message I am
> responding to.  Have all the issues been resolved, or are there
> still some loose ends to be hashed out?
>
> Hoping that the answer is "the former", I'll mark this topic as
> "Waiting for a reroll."
>
> Thanks.

As far as I know they have. You mentioned a few nits in one comment,
but then said you had a re-worded message that you queued so I assumed
those nits were all squashed in. If that's correct, and you already
applied my squash for the strbuf leak I think we're good and I had
nothing coming in a future re-roll based on looking at what you have
queued in your jk/diff-submodule-inline-diff branch.

Regards,
Jake

Re: [PATCH v13 14/14] builtin/am: use apply API in run_apply()

[Stefan Beller]

On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder
 wrote:
> +
> +   if (opts_left != 0)
> +   die("unknown option passed thru to git apply");

Through and thru are different spellings of the same word.
Thru is the less preferred form, however, and it might be
considered out of place outside the most informal contexts.

[Source: The Internet]

"git grep thru" confirms we only use it in comments or function
names, both are not exposed to our dear users.

The rest looks good.

Re: [PATCH v13 10/14] apply: change error_routine when silent

[Stefan Beller]

On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder
 wrote:
> To avoid printing anything when applying with
> `state->apply_verbosity == verbosity_silent`, let's save the
> existing warn and error routines before applying, and let's
> replace them with a routine that does nothing.
>
> Then after applying, let's restore the saved routines.
>
> Helped-by: Stefan Beller 
> Signed-off-by: Christian Couder 
> ---
>  apply.c | 21 -
>  apply.h |  8 
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/apply.c b/apply.c
> index ddbb0a2..bf81b70 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -112,6 +112,11 @@ void clear_apply_state(struct apply_state *state)
> /* >fn_table is cleared at the end of apply_patch() */
>  }
>
> +static void mute_routine(const char *bla, va_list params)

Instead of 'bla' you could go with 'format' as the man page for
[f]printf puts it.
Or you could leave it empty, i.e.

static void mute_routine(const char *, va_list)
...

I personally do not mind bla, as I know that the first parameter is
supposed to be the format, but let's not add unneeded information.
(Side question: Is there a culture that doesn't recognize 'bla' as a
fill word?)



> +{
> +   /* do nothing */
> +}
> +
>  int check_apply_state(struct apply_state *state, int force_apply)
>  {
> int is_not_gitdir = !startup_info->have_repository;
> @@ -144,6 +149,13 @@ int check_apply_state(struct apply_state *state, int 
> force_apply)
> if (!state->lock_file)
> return error("BUG: state->lock_file should not be NULL");
>
> +   if (state->apply_verbosity <= verbosity_silent) {
> +   state->saved_error_routine = get_error_routine();
> +   state->saved_warn_routine = get_warn_routine();
> +   set_error_routine(mute_routine);
> +   set_warn_routine(mute_routine);
> +   }
> +
> return 0;
>  }
>
> @@ -4864,7 +4876,7 @@ int apply_all_patches(struct apply_state *state,
> state->newfd = -1;
> }
>
> -   return !!errs;
> +   res = !!errs;

I am trying to understand this and the next chunk (they work together?)

>
>  end:
> if (state->newfd >= 0) {
> @@ -4872,5 +4884,12 @@ int apply_all_patches(struct apply_state *state,
> state->newfd = -1;
> }
>
> +   if (state->apply_verbosity <= verbosity_silent) {
> +   set_error_routine(state->saved_error_routine);
> +   set_warn_routine(state->saved_warn_routine);
> +   }
> +
> +   if (res > -1)
> +   return res;
> return (res == -1 ? 1 : 128);

So anything > -1 is returned as is, and == -1 returns 1 and <-1  returns 128 ?

So I guess a reminder/explanation on why we need to fiddle with return codes
in the commit message would help. (I do not understand how the
verbosity influences return codes.)

Re: [PATCH v13 00/14] libify apply and use lib in am, part 3

[Junio C Hamano]

Christian Couder  writes:

> On Mon, Aug 29, 2016 at 9:04 PM, Junio C Hamano  wrote:
>> Christian Couder  writes:
>>
>>> Highlevel view of the patches in the series
>>> ~~~
>>>
>>> This is "part 3" of the full patch series. I am resending only the
>>> last 14 patches of the full series as "part 3", because I don't want
>>> to resend the first 27 patches of v10 nearly as is.
>>
>> Just to make sure, you are sending the first 11 of these 14 exactly
>> as-is, right?  I didn't spot anything different other than 12 and 13
>> that replaced the "alternate index" step from the previous round.
>
> Yeah, the first 11 of the 14 patches have no change compared to v12.
> I didn't want to create a "part 4" as that could be confusing, and
> sending the first 11 patches gives them another chance to be reviewed
> again.

Hmph.

But most likely, you made sure that those who _could_ review the
first 11 are miniscule minority by omitting the earlier steps before
these 14 patches -- unless they are familiar with them, the first 11
patches are not much use to them.  And those who are familiar have
already seen the first 11, too.  That was why I wondered who the
target audience was when seeing only the last 14, among which 11 of
them were identical to the previous.

Re: [PATCH V2] git-send-email: Add ability to cc: any "bylines" in the commit message

[Jeff Kirsher]

On Wed, 2016-08-31 at 11:51 -0700, Joe Perches wrote:
> Many commits have various forms of bylines similar to
>  "Acked-by: Name " and "Reported-by: Name "
> 
> Add the ability to cc: bylines (e.g. Acked-by:) when using git send-
> email.
> 
> This can be suppressed with --suppress-cc=bylines.
> 
> Signed-off-by: Joe Perches 
> ---
>  Documentation/git-send-email.txt | 11 +++
>  git-send-email.perl  | 16 +++-
>  2 files changed, 18 insertions(+), 9 deletions(-)

Acked-by: Jeff Kirsher 

signature.asc
Description: This is a digitally signed message part

Re: [PATCH v13 06/14] apply: make it possible to silently apply

[Stefan Beller]

> Printing on stdout, and calls to warning() or error() are not
> taken care of in this patch, as that will be done in following
> patches.

> -   if (state->apply_verbosely)
> +   if (state->apply_verbosity > verbosity_normal)
> error(_("while searching for:\n%.*s"),
>   (int)(old - oldlines), oldlines);

But this is an error(..) ?

Have you considered to replace all these print functions (error, warning,
fprintf, printf, fprintf_ln) with another custom

int say_when_at_least(verbosity level, const char *fmt,...)

? (I guess that would be more invasive, but the result would be more
consistent.)

Re: [PATCH v13 05/14] apply: use error_errno() where possible

[Stefan Beller]

On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder
 wrote:
> To avoid possible mistakes and to uniformly show the errno
> related messages, let's use error_errno() where possible.
>
> Signed-off-by: Christian Couder 
> ---

Looks good,
Thanks for these cleanups!
Stefan

Re: [PATCH v13 04/14] apply: make some parsing functions static again

[Stefan Beller]

On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder
 wrote:
> Some parsing functions that were used in both "apply.c" and
> "builtin/apply.c" are now only used in the former, so they
> can be made static to "apply.c".
>
> Signed-off-by: Christian Couder 
> ---
>  apply.c | 6 +++---
>  apply.h | 5 -
>  2 files changed, 3 insertions(+), 8 deletions(-)

Looks good.

Re: [PATCH v13 03/14] Move libified code from builtin/apply.c to apply.{c,h}

[Stefan Beller]

On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder
 wrote:
> As most of the apply code in builtin/apply.c has been libified by a number of
> previous commits, it can now be moved to apply.{c,h}, so that more code can
> use it.
>
> Helped-by: Nguyễn Thái Ngọc Duy 
> Helped-by: Ramsay Jones 
> Signed-off-by: Christian Couder 
> ---
>  apply.c | 4731 ++
>  apply.h |   19 +
>  builtin/apply.c | 4733 
> +--

I deduce by roughly the same line count in the .c files, it is just
moving files over.
(On my todo list I have an idea how to make reviewing patches like this easier.)

>  3 files changed, 4751 insertions(+), 4732 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 2eac3e3..7b96130 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1,5 +1,23 @@
> +/*
> + * apply.c
> + *
> + * Copyright (C) Linus Torvalds, 2005

We're very inconsistent with the intellectual property log.
Sometimes we have that at the top of a file, sometimes we don't
and rather point at the git history to find out who touched the code.
I'd rather use the history instead of having a bunch of copyright lines.
So maybe consider to drop this introductory comment as a
{preparatory, follow up} cleanup?

(This is also a nit that doesn't require a reroll on its own)

Re: [PATCH v13 02/14] apply: rename and move opt constants to apply.h

[Stefan Beller]

On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder
 wrote:
>  extern int check_apply_state(struct apply_state *state, int force_apply);
>

With greater scope comes greater responsibility. Nit of the day:
In case a reroll is necessary, consider putting a comment here.
(What are these constants? what do they control? How/where do I use them?)

> +#define APPLY_OPT_INACCURATE_EOF   (1<<0)
> +#define APPLY_OPT_RECOUNT  (1<<1)
> +

What's cooking in git.git (Aug 2016, #09; Wed, 31)

[Junio C Hamano]

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

Now that final set of l10n message files are in, hopefully we can
have an uneventful 2.10 final this weekend.  Knock wood.

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

--
[New Topics]

* bh/diff-highlight-graph (2016-08-31) 6 commits
  (merged to 'next' on 2016-08-31 at 523a15f)
 + diff-highlight: avoid highlighting combined diffs
 + diff-highlight: add multi-byte tests
 + diff-highlight: ignore test cruft
 + diff-highlight: add support for --graph output
 + diff-highlight: add failing test for handling --graph output
 + diff-highlight: add some tests

 "diff-highlight" script (in contrib/) learned to work better with
 "git log -p --graph" output.

 Will keep in 'next' during the rest of the cycle.


* jc/am-read-author-file (2016-08-30) 1 commit
 - am: refactor read_author_script()

 Extract a small helper out of the function that reads the authors
 script file "git am" internally uses.


* jk/test-lib-drop-pid-from-results (2016-08-30) 1 commit
 - test-lib: drop PID from test-results/*.count

 The test framework left the number of tests and success/failure
 count in the t/test-results directory, keyed by the name of the
 test script plus the process ID.  The latter however turned out not
 to serve any useful purpose.  The process ID part of the filename
 has been removed.

 Will hold for a bit more and then merge to 'next'.


* js/sequencer-wo-die (2016-08-29) 14 commits
 - sequencer: lib'ify save_opts()
 - sequencer: lib'ify save_todo()
 - sequencer: lib'ify save_head()
 - sequencer: lib'ify create_seq_dir()
 - sequencer: lib'ify read_populate_opts()
 - sequencer: lib'ify read_populate_todo()
 - sequencer: lib'ify read_and_refresh_cache()
 - sequencer: lib'ify prepare_revs()
 - sequencer: lib'ify walk_revs_populate_todo()
 - sequencer: lib'ify do_pick_commit()
 - sequencer: lib'ify do_recursive_merge()
 - sequencer: lib'ify write_message()
 - sequencer: do not die() in do_pick_commit()
 - sequencer: lib'ify sequencer_pick_revisions()

 Lifts calls to exit(2) and die() higher in the callchain in
 sequencer.c files so that more helper functions in it can be used
 by callers that want to handle error conditions themselves.

 Waiting for the review discussion to settle.


* jk/squelch-false-warning-from-gcc-o3 (2016-08-31) 2 commits
 - color_parse_mem: initialize "struct color" temporary
 - error_errno: use constant return similar to error()

--
[Graduated to "master"]

* bw/mingw-avoid-inheriting-fd-to-lockfile (2016-08-23) 2 commits
  (merged to 'next' on 2016-08-24 at 8389b0e)
 + mingw: ensure temporary file handles are not inherited by child processes
 + t6026-merge-attr: child processes must not inherit index.lock handles

 The tempfile (hence its user lockfile) API lets the caller to open
 a file descriptor to a temporary file, write into it and then
 finalize it by first closing the filehandle and then either
 removing or renaming the temporary file.  When the process spawns a
 subprocess after obtaining the file descriptor, and if the
 subprocess has not exited when the attempt to remove or rename is
 made, the last step fails on Windows, because the subprocess has
 the file descriptor still open.  Open tempfile with O_CLOEXEC flag
 to avoid this (on Windows, this is mapped to O_NOINHERIT).


* dg/document-git-c-in-git-config-doc (2016-08-23) 1 commit
  (merged to 'next' on 2016-08-24 at a923eb0)
 + doc: mention `git -c` in git-config(1)

 The "git -c var[=val] cmd" facility to append a configuration
 variable definition at the end of the search order was described in
 git(1) manual page, but not in git-config(1), which was more likely
 place for people to look for when they ask "can I make a one-shot
 override, and if so how?"


* ja/i18n (2016-08-24) 3 commits
  (merged to 'next' on 2016-08-24 at 3a084aa)
 + i18n: simplify numeric error reporting
 + i18n: fix git rebase interactive commit messages
 + i18n: fix typos for translation

 The recent i18n patch we added during this cycle did a bit too much
 refactoring of the messages to avoid word-legos; the repetition has
 been reduced to help translators.


* js/no-html-bypass-on-windows (2016-08-19) 1 commit
  (merged to 'next' on 2016-08-24 at 05728f8)
 + Revert "display HTML in default browser using Windows' shell API"

 On Windows, help.browser configuration variable used to be ignored,
 which has been corrected.


* kw/patch-ids-optim (2016-08-29) 1 commit
  (merged to 'next' on 2016-08-30 at 9590e42)
 + p3400: make test script executable

 Micro hot-fix for a topic that 

Re: Literate programming with git

[Stefan Beller]

On Wed, Aug 31, 2016 at 1:41 PM, Ben North  wrote:
>
> https://github.com/bennorth/git-dendrify

So looking at the Readme there:

* Add printing facility
|\
| * Add watermarks
| |\
| | * Allow choice of colour
| | * Add known-good test cases
| | * Emit watermark 'underneath' main output
| | * Drop-down for common watermarks
| |/
| * Add actual printing via PDF
| |\
| | * Submit PDF to system print service
...

This reminds me of the workflow of git itself.
(As a literate consumer) You get an easy top-level overview what
the community is interested in via e.g.:

git log --first-parent --oneline

That would be equivalent to showing only
* Add printing facility

If you run that command on "* Add printing facility"^2
you would see the headlines of the section.

However in gits reality we do not have these nice sections
building on top of each other, as many people are interested in
different things and build where they see fit.

> The hierarchical organisation is helpful when reading the
> history, and also allows that history to be rendered into a structured
> document explaining the code's development.

How does the linearify/dendrify work with already non-linear history?

Thanks,
Stefan

Re: [PATCH 18/22] sequencer: support amending commits

[Jakub Narębski]

W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:

> This teaches the sequencer_commit() function to take an argument that
> will allow us to implement "todo" commands that need to amend the commit
> messages ("fixup", "squash" and "reword").
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 6 --
>  sequencer.h | 2 +-
>  2 files changed, 5 insertions(+), 3 deletions(-)

Nice and small addition of a new feature, a scaffolding for implementing
rebase -i using the sequencer.

> 
> diff --git a/sequencer.c b/sequencer.c
> index 7e17d14..20f7590 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -478,7 +478,7 @@ static char **read_author_script(void)
>   * (except, of course, while running an interactive rebase).
>   */
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -   int allow_empty, int edit)
> +   int allow_empty, int edit, int amend)

I guess we won't get much more parameters; it would get unwieldy
(and hard to remember).  Five is all right.

>  {
>   char **env = NULL;
>   struct argv_array array;
> @@ -507,6 +507,8 @@ int sequencer_commit(const char *defmsg, struct 
> replay_opts *opts,
>   argv_array_push(, "commit");
>   argv_array_push(, "-n");
>  
> + if (amend)
> + argv_array_push(, "--amend");
>   if (opts->gpg_sign)
>   argv_array_pushf(, "-S%s", opts->gpg_sign);
>   if (opts->signoff)
> @@ -779,7 +781,7 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   }
>   if (!opts->no_commit)
>   res = sequencer_commit(opts->edit ? NULL : git_path_merge_msg(),
> - opts, allow, opts->edit);
> + opts, allow, opts->edit, 0);

... even of this makes one need to check the calling convention,
what does this 0 mean.

>  
>  leave:
>   free_message(commit, );
> diff --git a/sequencer.h b/sequencer.h
> index fd02baf..2106c0d 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -50,7 +50,7 @@ int sequencer_rollback(struct replay_opts *opts);
>  int sequencer_remove_state(struct replay_opts *opts);
>  
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -   int allow_empty, int edit);
> +   int allow_empty, int edit, int amend);
>  
>  extern const char sign_off_header[];
>  
> 

Re: [PATCH 17/22] sequencer: allow editing the commit message on a case-by-case basis

[Jakub Narębski]

W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:

> In the upcoming commits, we will implement more and more of rebase
> -i's functionality. One particular feature of the commands to come is
> that some of them allow editing the commit message while others don't,
> i.e. we cannot define in the replay_opts whether the commit message
> should be edited or not.

It's a nice, pretty and self contained refactoring step.  Small
enough that it is easy to review.

I would like to have in the commit message that it is sequencer_commit()
function that needs to rely on new parameter, instead of on a property
of command (of its replay_opts).  And that currently it simply passes
the buck to caller, which uses opts->edit, but in the future the
caller that is rebase -i would use todo_item and replay_opts based
expression.

> 
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 6 +++---
>  sequencer.h | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index e094ac2..7e17d14 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -478,7 +478,7 @@ static char **read_author_script(void)
>   * (except, of course, while running an interactive rebase).
>   */
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -   int allow_empty)
> +   int allow_empty, int edit)
>  {
>   char **env = NULL;
>   struct argv_array array;
> @@ -513,7 +513,7 @@ int sequencer_commit(const char *defmsg, struct 
> replay_opts *opts,
>   argv_array_push(, "-s");
>   if (defmsg)
>   argv_array_pushl(, "-F", defmsg, NULL);
> - if (opts->edit)
> + if (edit)
>   argv_array_push(, "-e");
>   else if (!opts->signoff && !opts->record_origin &&
>git_config_get_value("commit.cleanup", ))
> @@ -779,7 +779,7 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   }
>   if (!opts->no_commit)
>   res = sequencer_commit(opts->edit ? NULL : git_path_merge_msg(),
> - opts, allow);
> + opts, allow, opts->edit);
>  
>  leave:
>   free_message(commit, );
> diff --git a/sequencer.h b/sequencer.h
> index 9f63c31..fd02baf 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -50,7 +50,7 @@ int sequencer_rollback(struct replay_opts *opts);
>  int sequencer_remove_state(struct replay_opts *opts);
>  
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -   int allow_empty);
> +   int allow_empty, int edit);
>  
>  extern const char sign_off_header[];
>  
> 

Re: [PATCH v2 2/4] cat-file: introduce the --filters option

[Junio C Hamano]

Johannes Schindelin  writes:

>> > +  if (S_ISREG(mode)) {
>> > +  struct strbuf strbuf = STRBUF_INIT;
>> > +  if (convert_to_working_tree(path, *buf, *size, )) {
>> > +  free(*buf);
>> > +  *size = strbuf.len;
>> > +  *buf = strbuf_detach(, NULL);
>> > +  }
>> > +  }
>> 
>> When we see a blob that is not ISREG, what is expected to happen?
>> Is it an error?
>
> This is not a user-facing command, therefore we have to trust the caller
> that they know what they are doing.

The caller that knows what s/he is doing would rely on a documented
behaviour out of the command.  That behaviour hopefully is an
intuitive and useful one for script writers.

You say

> Quite frankly, as cat-file is not an end-user-facing command, I think it
> is serious overkill to add more testing here.

and I think you would need a serious attitude adjustment here.
Scriptors are also an important class of end-users and cat-file
directly faces them.

Thinking about this one a bit more, as 'cat-file' especially with
the "--filters" option is the lowest-level way for scriptors to
externalize the data stored in Git object database to the
representation used in the outside world (in other words, it would
be a good ingredient if they want to implement what "checkout"
does), I would expect that an intuitive behaviour for

git cat-file --filters HEAD:Makefile >Makefile
git cat-file --filters HEAD:RelNotes >Relnotes
git cat-file --filters HEAD:t >t

to send the requested contents to the standard output stream, but
error out when the result of the command shown above would not
mimick what "checkout" would leave in the filesystem.  IOW, the
first one should succeed, the second and the third ones should fail
the same way to signal what is requested cannot be performed to the
script that is calling these commands.

Re: [PATCH 05/34] sequencer (rebase -i): learn about the 'verbose' mode

[Johannes Schindelin]

Hi Dennis,

On Wed, 31 Aug 2016, Dennis Kaarsemaker wrote:

> On wo, 2016-08-31 at 10:54 +0200, Johannes Schindelin wrote:
> 
> > +   if (file_exists(rebase_path_verbose()))
> > +   opts->verbose = 1;
> 
> I don't see anything in this series that creates this file, will that
> be part of a later series?

No. The sequencer does not write that file, but just consumes it. `git
rebase` writes it.

Ciao,
Dscho

Literate programming with git

[Ben North]

Hi,

I've recently been experimenting with using git to make software more
human-readable.  Presenting software for humans to read is not a new
idea (Knuth's 'literate programming'), but I think git can be a new
tool for showing the development of code in a structured way.
Merge-commits can break a flat sequence of commits into sections and
subsections, in the same way that a document's paragraphs are
arranged.  The hierarchical organisation is helpful when reading the
history, and also allows that history to be rendered into a structured
document explaining the code's development.

As a demo, I've created:

http://www.redfrontdoor.org/20160813-literate-git-demo/index.html

This was generated directly from the git repo of the project, using
tools I wrote:

https://github.com/bennorth/literate-git

For working with hierarchical git histories, I wrote another tool:

https://github.com/bennorth/git-dendrify

The READMEs of the two projects give more details of these ideas.

This is at the prototype / proof-of-concept stage --- any feedback welcome!

Thanks,

Ben.

Re: [PATCH v2 0/4] cat-file: optionally convert to worktree version

[Johannes Schindelin]

Hi Junio,

On Mon, 29 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > When third-party tools need to access to contents of blobs in the
> > database, they might be more interested in the worktree version than in
> > the "clean" version of said contents.
> 
> Just a friendly reminder before you completely shift your attention
> to unrelated topics.  I think this topic is almost there; let's not
> stretch ourselves too thin by nibbling-here-nibbling-there and leaving
> loose ends untied.

I answered the comments and changed one thing locally. Will send out v3
tomorrow.

Ciao,
Dscho

Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings

[Junio C Hamano]

Jakub Narębski  writes:

> Though xstrdup(buf.buf + 2) followed by strbuf_release() would
> make free(opts->gpg_sign) possible without crash.  That is we can
> work without *_entrust() mechanism at the cost of strdups.

Absolutely.

It is not like entrust() thing is free of allocation cost (it needs
to allocate an array of pointers to keep track of what to free) or
programmer's mental burden (you need to be careful what to entrust()
and what not to), so "at the cost of strdup(3)" is reasonable cost
of doing business in the way normal people expect the code to work.

Re: [PATCH v2 2/4] cat-file: introduce the --filters option

[Johannes Schindelin]

Hi Junio,

On Wed, 24 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > +static int filter_object(const char *path, unsigned mode,
> > +const unsigned char *sha1,
> > +char **buf, unsigned long *size)
> > +{
> > +   enum object_type type;
> > +
> > +   *buf = read_sha1_file(sha1, , size);
> > +   if (!*buf)
> > +   return error(_("cannot read object %s '%s'"),
> > +   sha1_to_hex(sha1), path);
> > +   if (type != OBJ_BLOB) {
> > +   free(*buf);
> > +   return error(_("blob expected for %s '%s'"),
> > +   sha1_to_hex(sha1), path);
> > +   }
> > +   if (S_ISREG(mode)) {
> > +   struct strbuf strbuf = STRBUF_INIT;
> > +   if (convert_to_working_tree(path, *buf, *size, )) {
> > +   free(*buf);
> > +   *size = strbuf.len;
> > +   *buf = strbuf_detach(, NULL);
> > +   }
> > +   }
> 
> When we see a blob that is not ISREG, what is expected to happen?
> Is it an error?

This is not a user-facing command, therefore we have to trust the caller
that they know what they are doing.

And it is quite conceivable that a caller wants to simply apply filters
whenever a blob is specified, and simply not apply any filters when
something else was specified.

That would be in line with specifying the --filters options on a path for
which no filters are configured: the --filters option is basically a
no-op, then.

> In any case, silently succeeding without any output is probably what
> the end-user least expects.

Except that 1) the command is not for an end-user, and 2) when calling
`git cat-file --filters HEAD:directory/` the command does not silently
succeed:

error: blob expected for  'directory/'

> If we choose to fail the request, the necessary update to the test
> would look like this, I think.

Quite frankly, as cat-file is not an end-user-facing command, I think it
is serious overkill to add more testing here.

Ciao,
Dscho

Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings

[Jakub Narębski]

W dniu 31.08.2016 o 22:12, Junio C Hamano pisze:
> Jakub Narębski  writes:
>> Johannes Schindelin wrote:

>>> +   else {
>>> +   opts->gpg_sign = buf.buf + 2;
>>> +   strbuf_detach(, NULL);
>>
>> Wouldn't we leak 2 characters that got skipped?  Maybe xstrdup would
>> be better (if it is leaked, and not reattached)?
> 
> An attempt to avoid leaking by calling free(opts->gpg_sign) would
> make it crash, which would be even worse ;-).
 
Actually, from C standard:

"If ptr [in free(ptr)] does not point to a block of memory allocated
 with the above functions [malloc(), etc.], it causes undefined behavior."
  ^

Which is even worse if it does not lead to crash.


Note that if the last line was

+   sequencer_entrust(opts, strbuf_detach(, 
NULL));

we can make it not leak.  A bit tricksy, though.


Though xstrdup(buf.buf + 2) followed by strbuf_release() would
make free(opts->gpg_sign) possible without crash.  That is we can
work without *_entrust() mechanism at the cost of strdups.

-- 
Jakub Narębski

Re: [PATCH v2 3/4] cat-file --textconv/--filters: allow specifying the path separately

[Junio C Hamano]

Johannes Schindelin  writes:

> However, I do not want to hold this patch series up just by being
> stubborn.

Peff and I disussed this further in the thread in which the message
 appears, and agreed
that it does not matter that much either way.

The comment to [v2 2/4] would be more important than this one, I
would think.  What should happen when a blob that is not ISREG is
given to filter_object()?  Giving a symlink contents as-is without
filtering would be OK.  Erroring out saying "regular file blob
expected" just like the function reacts to non-blob is also OK.
Just being silent and not doing anything is probably not.

Thanks.

Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings

[Junio C Hamano]

Jakub Narębski  writes:

>> +else {
>> +opts->gpg_sign = buf.buf + 2;
>> +strbuf_detach(, NULL);
>
> Wouldn't we leak 2 characters that got skipped?  Maybe xstrdup would
> be better (if it is leaked, and not reattached)?

An attempt to avoid leaking by calling free(opts->gpg_sign) would
make it crash, which would be even worse ;-).

Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings

[Jakub Narębski]

Hello Johannes,

W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:

> The rebase command sports a `--gpg-sign` option that is heeded by the
> interactive rebase.

Should it be "sports" or "supports"?

> 
> This patch teaches the sequencer that trick, as part of the bigger
> effort to make the sequencer the work horse of the interactive rebase.
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 48 +++-
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 4204cc8..e094ac2 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -15,6 +15,7 @@
>  #include "merge-recursive.h"
>  #include "refs.h"
>  #include "argv-array.h"
> +#include "quote.h"
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> @@ -33,6 +34,11 @@ static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
>   * being rebased.
>   */
>  static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
> +/*
> + * The following files are written by git-rebase just after parsing the
> + * command-line (and are only consumed, not modified, by the sequencer).
> + */

It is good to have this comment here.

> +static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")

I know it is not your fault, but I wonder why this file uses
snake_case_name, while all other use kebab-case-names.  That is,
why it is gpg_sign_opt and not gpg-sign-opt.

>  
>  /* We will introduce the 'interactive rebase' mode later */
>  #define IS_REBASE_I() 0
> @@ -129,6 +135,16 @@ static int has_conforming_footer(struct strbuf *sb, 
> struct strbuf *sob,
>   return 1;
>  }
>  
> +static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
> +{
> + static struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_reset();
> + if (opts->gpg_sign)
> + sq_quotef(, "-S%s", opts->gpg_sign);
> + return buf.buf;
> +}

All right, this function is quite clear.

Sidenote: it's a pity api-quote.txt is just a placeholder for proper
documentation (including sq_quotef()).  I also wonder why it is not
named sq_quotef_buf() or strbuf_addf_sq().

> +
>  void *sequencer_entrust(struct replay_opts *opts, void 
> *set_me_free_after_use)
>  {
>   ALLOC_GROW(opts->owned, opts->owned_nr + 1, opts->owned_alloc);
> @@ -471,17 +487,20 @@ int sequencer_commit(const char *defmsg, struct 
> replay_opts *opts,
>  
>   if (IS_REBASE_I()) {
>   env = read_author_script();
> - if (!env)
> + if (!env) {
> + const char *gpg_opt = gpg_sign_opt_quoted(opts);
> +
>   return error("You have staged changes in your working "
>   "tree. If these changes are meant to be\n"
>   "squashed into the previous commit, run:\n\n"
> - "  git commit --amend $gpg_sign_opt_quoted\n\n"

How did this get expanded by error(), and why we want to replace
it if it works?

> + "  git commit --amend %s\n\n"
>   "If they are meant to go into a new commit, "
>   "run:\n\n"
> - "  git commit $gpg_sign_opt_quoted\n\n"
> + "  git commit %s\n\n"
>   "In both case, once you're done, continue "
>   "with:\n\n"
> - "  git rebase --continue\n");
> + "  git rebase --continue\n", gpg_opt, gpg_opt);

Instead of passing option twice, why not make use of %1$s (arg reordering),
that is

  + "  git commit --amend %1$s\n\n"
[...]
  + "  git commit %1$s\n\n"


> + }

So shell quoting is required only for error output.

>   }
>  
>   argv_array_init();
> @@ -955,8 +974,27 @@ static int populate_opts_cb(const char *key, const char 
> *value, void *data)
>  
>  static int read_populate_opts(struct replay_opts *opts)
>  {
> - if (IS_REBASE_I())
> + if (IS_REBASE_I()) {
> + struct strbuf buf = STRBUF_INIT;
> +
> + if (read_oneliner(, rebase_path_gpg_sign_opt(), 1)) {
> + if (buf.len && buf.buf[buf.len - 1] == '\n') {
> + if (--buf.len &&
> + buf.buf[buf.len - 1] == '\r')
> + buf.len--;
> + buf.buf[buf.len] = '\0';
> + }

Isn't there some strbuf_chomp() / strbuf_strip_eof() function?
Though as strbuf_getline() uses something similar...

> +
> + if (!starts_with(buf.buf, "-S"))
> + strbuf_reset();

Should we signal that there was problem with a file contents?

> + else {
> + opts->gpg_sign = buf.buf 

Re: [PATCH V2] git-send-email: Add ability to cc: any "bylines" in the commit message

[Junio C Hamano]

Joe Perches  writes:

> On Wed, 2016-08-31 at 12:34 -0700, Junio C Hamano wrote:
>> Joe Perches  writes:
>> > Many commits have various forms of bylines similar to
>> A missing blank line (I can tweak while queuing).
> []
>> > +  next if $suppress_cc{'bylines'} and $what !~ 
>> > /Signed-off-by/i and $what =~ /by$/i;
>> Having to keep this /by$/i in sync with whatever definition of
>> bylines is will be error prone.  How about doing it in this way?
>> 
>>  # Now parse the message body
>> +my $bypat = r/[\w-]+-by/;
>>  while (<$fh>) {
>>  ...
>> if (/^(Signed-off-by|Cc|$bypat): (.*)$/i) {
>>  ...
>>  next if $suppress_cc{'bodycc'} and $what =~ 
>> /Cc/i;
>> +next if $suppress_cc{'bylines'} and
>> +$what !~ /^Signed-off-by/i and
>> +$what =~ /^$bypat/i;
>> 
>> Other than that, looking good.
>
> Sure, whatever you want, do you want a v3 from me or can
> you fix it up however you want?

This topic is not my itch, so "however I want" would not be a good
instruction to me--The lazy one in me would be tempted to say "ok,
then I'd drop it altogether" ;-)

I am sure the typo "[^\s]+[\w-]by" in the one we just saw was merely
because you rushed it out without double checking.  We are in
pre-release feature freeze so there is no need to rush.  I'd prefer
to see a final version that is carefully proof-read by the author.

Thanks.

Re: [PATCH 0/2] squelch some "gcc -O3 -Wmaybe-uninitialized" warnings

[Junio C Hamano]

Jeff King  writes:

> I happened to be compiling git with -O3 today, and noticed we generate
> some warnings about uninitialized variables (I actually compile with
> -Wall, but the only false positives I saw were these).
>
> Here are patches to squelch them.
>
>   [1/2]: error_errno: use constant return similar to error()
>   [2/2]: color_parse_mem: initialize "struct color" temporary

Interesting.  Here is for "gcc -Os" on top to appease gcc 4.8.4 that
I probably am NOT going to apply.  These are all false positives.

The ones on config.c is the most curious as these two "ret" needs a
false initialization, but the one that comes after them
git_config_ulong() that has the same code structure does not get any
warning, which made absolutely no sense to me.


 builtin/update-index.c | 2 +-
 config.c   | 4 ++--
 diff.c | 2 +-
 fast-import.c  | 1 +
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index ba04b19..a202612 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -832,7 +832,7 @@ static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,
const struct option *opt, int unset)
 {
unsigned char sha1[20];
-   unsigned int mode;
+   unsigned int mode = 0;
const char *path;
 
if (!parse_new_style_cacheinfo(ctx->argv[1], , sha1, )) {
diff --git a/config.c b/config.c
index 0dfed68..52133aa 100644
--- a/config.c
+++ b/config.c
@@ -685,7 +685,7 @@ static void die_bad_number(const char *name, const char 
*value)
 
 int git_config_int(const char *name, const char *value)
 {
-   int ret;
+   int ret = 0;
if (!git_parse_int(value, ))
die_bad_number(name, value);
return ret;
@@ -693,7 +693,7 @@ int git_config_int(const char *name, const char *value)
 
 int64_t git_config_int64(const char *name, const char *value)
 {
-   int64_t ret;
+   int64_t ret = 0;
if (!git_parse_int64(value, ))
die_bad_number(name, value);
return ret;
diff --git a/diff.c b/diff.c
index c7e2605..1098198 100644
--- a/diff.c
+++ b/diff.c
@@ -995,7 +995,7 @@ static int find_word_boundaries(mmfile_t *buffer, regex_t 
*word_regex,
 static void diff_words_fill(struct diff_words_buffer *buffer, mmfile_t *out,
regex_t *word_regex)
 {
-   int i, j;
+   int i, j = 0;
long alloc = 0;
 
out->size = 0;
diff --git a/fast-import.c b/fast-import.c
index bf53ac9..abc4519 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1377,6 +1377,7 @@ static const char *get_mode(const char *str, uint16_t 
*modep)
unsigned char c;
uint16_t mode = 0;
 
+   *modep = 0;
while ((c = *str++) != ' ') {
if (c < '0' || c > '7')
return NULL;

Re: [PATCH v2 3/4] cat-file --textconv/--filters: allow specifying the path separately

[Johannes Schindelin]

Hi Junio,

On Wed, 24 Aug 2016, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Mentioned elsewhere, but I think the above should be
> >
> > if (!path)
> > path = obj_context.path;
> >
> > if (obj_context.mode == S_IFINVALID)
> > obj_context.mode = 0100644;
> >
> > IOW, even when there is an explicit path supplied, we should fall
> > back to assumed "regular blob" mode, so that
> >
> > git cat-file --filters --path=README $(git rev-parse :README)
> >
> > would work as expected.
> 
> Actually, I am reading the conditional the other way, but the
> conclusion "defaulting from unknown mode to regular blob is
> necessary whether the user gave us a path or not" is the same.
> 
> The current code may fail if --path is not available and 40-hex that
> does not give us any context of look up is given because it won't be
> able to decide how to filter, so using "else if" would not have
> practical difference there, but conceptually it still is wrong.

Let's translate the logic

if (!path)
path = obj_context.path;
else if (obj_context.mode == S_IFINVALID)
obj_context.mode = 0100644;

into plain English.

Basically, we have two cases:

1) the user provided us with a --path option, in which case we only
   override the mode if get_sha1_with_context() could not determine the
   mode.

2) the user did *not* provide us with a --path, in which case we keep the
   mode exactly as determined by get_sha1_with_context().

Now, the change you propose would change 2) such that we would *still*
override an undecided mode with 100644, even if the user did not bother to
specify a --path option.

It is true that it is currently impossible to infer a path from a blob ID
without being able to infer also a mode. So the question is whether it
makes sense to allow cat-file to proceed on the assumption that it is a
regular file if it does not know?

I would say: no, it does not make sense.

However, I do not want to hold this patch series up just by being
stubborn.

Will change it,
Dscho

Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

[Junio C Hamano]

Jakub Narębski  writes:

 @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts 
 *opts)
  struct todo_item {
enum todo_command command;
struct commit *commit;
 +  const char *arg;
 +  int arg_len;
>  
>> I am not sure what the "commit" field of type "struct commit *" is
>> for.  It is not needed until it is the commit's turn to be picked or
>> reverted; if we end up stopping in the middle, parsing the commit
>> object for later steps will end up being wasted effort.
>
> From what I understand this was what sequencer did before this
> series, so it is not a regression (I think; the commit parsing
> was in different function, but I think at the same place in
> the callchain).

Yes, I agree with you and I didn't mean "this is a new bug" at all.
It just is an indication that further refactoring after this step is
needed, and it is likely to involve removal or modification of this
field.

Re: [PATCH V2] git-send-email: Add ability to cc: any "bylines" in the commit message

[Joe Perches]

On Wed, 2016-08-31 at 12:34 -0700, Junio C Hamano wrote:
> Joe Perches  writes:
> > Many commits have various forms of bylines similar to
> A missing blank line (I can tweak while queuing).
[]
> > +   next if $suppress_cc{'bylines'} and $what !~ 
> > /Signed-off-by/i and $what =~ /by$/i;
> Having to keep this /by$/i in sync with whatever definition of
> bylines is will be error prone.  How about doing it in this way?
> 
>   # Now parse the message body
> + my $bypat = r/[\w-]+-by/;
>   while (<$fh>) {
>   ...
> if (/^(Signed-off-by|Cc|$bypat): (.*)$/i) {
>   ...
>   next if $suppress_cc{'bodycc'} and $what =~ 
> /Cc/i;
> + next if $suppress_cc{'bylines'} and
> + $what !~ /^Signed-off-by/i and
> + $what =~ /^$bypat/i;
> 
> Other than that, looking good.

Sure, whatever you want, do you want a v3 from me or can
you fix it up however you want?

Re: [PATCH V2] git-send-email: Add ability to cc: any "bylines" in the commit message

[Junio C Hamano]

Joe Perches  writes:

> Many commits have various forms of bylines similar to

A missing blank line (I can tweak while queuing).

>  "Acked-by: Name " and "Reported-by: Name "
>
> Add the ability to cc: bylines (e.g. Acked-by:) when using git send-email.
>
> This can be suppressed with --suppress-cc=bylines.
> ...
> @@ -307,8 +308,10 @@ Automating
>patch body (commit message) except for self (use 'self' for that).
>  - 'sob' will avoid including anyone mentioned in Signed-off-by lines except
> for self (use 'self' for that).
> +- 'bylines' will avoid including anyone mentioned in any "-by:" lines
> +  in the patch header except for Signed-off-by.

 feels a bit too informal but I don't think of a better
alternative, perhaps other than "*-by:".

> @@ -1545,7 +1545,7 @@ foreach my $t (@files) {
>   # Now parse the message body
>   while(<$fh>) {
>   $message .=  $_;
> - if (/^(Signed-off-by|Cc): (.*)$/i) {
> + if (/^(Signed-off-by|Cc|[^\s]+[\w-]by): (.*)$/i) {

I thought you wanted

if (/^(Signed-off-by|Cc|[\w-]+-by): (.*)$/i) {

instead to avoid "O_=:;fooby: Joe Perches "
>   chomp;
>   my ($what, $c) = ($1, $2);
>   chomp $c;
> @@ -1555,6 +1555,12 @@ foreach my $t (@files) {
>   } else {
>   next if $suppress_cc{'sob'} and $what =~ 
> /Signed-off-by/i;
>   next if $suppress_cc{'bodycc'} and $what =~ 
> /Cc/i;
> + next if $suppress_cc{'bylines'} and $what !~ 
> /Signed-off-by/i and $what =~ /by$/i;

Having to keep this /by$/i in sync with whatever definition of
bylines is will be error prone.  How about doing it in this way?

# Now parse the message body
+   my $bypat = r/[\w-]+-by/;
while (<$fh>) {
...
if (/^(Signed-off-by|Cc|$bypat): (.*)$/i) {
...
next if $suppress_cc{'bodycc'} and $what =~ 
/Cc/i;
+   next if $suppress_cc{'bylines'} and
+   $what !~ /^Signed-off-by/i and
+   $what =~ /^$bypat/i;

Other than that, looking good.

Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

[Jakub Narębski]

W dniu 31.08.2016 o 21:10, Junio C Hamano pisze:
> Jakub Narębski  writes:
> 
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 06759d4..3398774 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts 
>>> *opts)
>>>  struct todo_item {
>>> enum todo_command command;
>>> struct commit *commit;
>>> +   const char *arg;
>>> +   int arg_len;
 
> I am not sure what the "commit" field of type "struct commit *" is
> for.  It is not needed until it is the commit's turn to be picked or
> reverted; if we end up stopping in the middle, parsing the commit
> object for later steps will end up being wasted effort.

>From what I understand this was what sequencer did before this
series, so it is not a regression (I think; the commit parsing
was in different function, but I think at the same place in
the callchain).

> 
> Also, when the sequencer becomes one sequencer to rule them all, the
> command set may contain something that does not even mention any
> commit at all (think: exec).

The "exec" line is a bit of exception, all other rebase -i commands
take commit as parameter.  It could always use NULL.

>
> So I am not sure if we want a parsed commit there (I would not
> object if we kept the texual object name read from the file,
> though).  The "one sequencer to rule them all" may even have to say
> "now give name ':1' to the result of the previous operation" in one
> step and in another later step have an instruction "merge ':1'".
> When that happens, you cannot even pre-populate the commit object
> when the sequencer reads the file, as the commit has not yet been
> created at that point.

True, --preserve-merges rebase is well, different.

Best,
-- 
Jakub Narebski

Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

[Junio C Hamano]

Jakub Narębski  writes:

>> diff --git a/sequencer.c b/sequencer.c
>> index 06759d4..3398774 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts 
>> *opts)
>>  struct todo_item {
>>  enum todo_command command;
>>  struct commit *commit;
>> +const char *arg;
>> +int arg_len;
>
> Why 'arg', and not 'oneline', or 'subject'?
> I'm not saying it is bad name.

I am not sure what the "commit" field of type "struct commit *" is
for.  It is not needed until it is the commit's turn to be picked or
reverted; if we end up stopping in the middle, parsing the commit
object for later steps will end up being wasted effort.

Also, when the sequencer becomes one sequencer to rule them all, the
command set may contain something that does not even mention any
commit at all (think: exec).

So I am not sure if we want a parsed commit there (I would not
object if we kept the texual object name read from the file,
though).  The "one sequencer to rule them all" may even have to say
"now give name ':1' to the result of the previous operation" in one
step and in another later step have an instruction "merge ':1'".
When that happens, you cannot even pre-populate the commit object
when the sequencer reads the file, as the commit has not yet been
created at that point.

Re: [PATCH 08/22] sequencer: remove overzealous assumption

[Junio C Hamano]

Johannes Schindelin  writes:

> The problem is that the `git-rebase-todo` most definitely does *not* want
> to be restricted to a single command.

Looking at the test in question, it is about what happens when "git
cherry-pick A..B" runs, and the test assumes it gets a sequence of
"pick".  It would be a bug to see any "revert" in there if you are
doing a multi cherry-pick.

And what is checked is if "cherry-pick --continue" notices a
malformed instruction that has something other than "pick".

If the final invocation were "sequencer --continue", then I
perfectly well understand why it is not a good idea to limit the set
of instructions only to "pick" (or "revert").  But I do not think
the test performed here is for that general case.  The user knows
Git stopped in the middle of cherry-pick and expects cherry-pick to
continue.

> So if you must have a patch that disagrees with this overzealous check,
> the "revamp todo parsing" one is probably the first. But it is better to
> think of this at a higher level than just patches: it is wrong to limit
> the todo script to contain only identical commands.

So if you think of this at even higher level, the check done in
parse_insn_line() that _assumes_ that opts->action must match the
actions on each line is _WRONG_, but what this test expects to see
is perfectly reasonable, I would think.

It is a different matter if it makes sense to _verify_ that the user
didn't make nonsensical change to the generated insn and error out
when s/he did.  I tend to think it is pointless, and I wouldn't object
if this test is removed due to that reason.

Re: git submodules implementation question

[Uma Srinivasan]

> We want to affect only the process we are going to spawn to work
> inside the submodule, not ourselves, which is what this call does;
> this does not sound like a good idea.

Okay, in that case I would have to pass the "git_dir" as a new
argument to prepare_submodule_repo_env(). I know what to pass from the
is_submodule_modified() caller. I don't think it's all that obvious
for the other callers.

Thanks,
Uma

On Wed, Aug 31, 2016 at 11:44 AM, Junio C Hamano  wrote:
> Uma Srinivasan  writes:
>
>> diff --git a/submodule.c b/submodule.c
>> index 5a62aa2..23443a7 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -960,6 +960,9 @@ unsigned is_submodule_modified(const char *path,
>> int ignore_untracked)
>> return 0;
>>
>> }
>> +   /* stuff submodule git dir into env var */
>> +   set_git_dir(git_dir);
>
> We want to affect only the process we are going to spawn to work
> inside the submodule, not ourselves, which is what this call does;
> this does not sound like a good idea.
>

[PATCH V2] git-send-email: Add ability to cc: any "bylines" in the commit message

[Joe Perches]

Many commits have various forms of bylines similar to
 "Acked-by: Name " and "Reported-by: Name "

Add the ability to cc: bylines (e.g. Acked-by:) when using git send-email.

This can be suppressed with --suppress-cc=bylines.

Signed-off-by: Joe Perches 
---
 Documentation/git-send-email.txt | 11 +++
 git-send-email.perl  | 16 +++-
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 642d0ef..0b0d945 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -278,9 +278,10 @@ Automating
the value of `sendemail.identity`.
 
 --[no-]signed-off-by-cc::
-   If this is set, add emails found in Signed-off-by: or Cc: lines to the
-   cc list. Default is the value of `sendemail.signedoffbycc` configuration
-   value; if that is unspecified, default to --signed-off-by-cc.
+   If this is set, add emails found in Signed-off-by: or Cc: or any other
+   byline (e.g. Acked-by:) lines to the cc list. Default is the value of
+   `sendemail.signedoffbycc` configuration value; if that is unspecified,
+   default to --signed-off-by-cc.
 
 --[no-]cc-cover::
If this is set, emails found in Cc: headers in the first patch of
@@ -307,8 +308,10 @@ Automating
   patch body (commit message) except for self (use 'self' for that).
 - 'sob' will avoid including anyone mentioned in Signed-off-by lines except
for self (use 'self' for that).
+- 'bylines' will avoid including anyone mentioned in any "-by:" lines
+  in the patch header except for Signed-off-by.
 - 'cccmd' will avoid running the --cc-cmd.
-- 'body' is equivalent to 'sob' + 'bodycc'
+- 'body' is equivalent to 'sob' + 'bodycc' + 'bylines'
 - 'all' will suppress all auto cc values.
 --
 +
diff --git a/git-send-email.perl b/git-send-email.perl
index da81be4..1f53328 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -84,7 +84,7 @@ git send-email --dump-aliases
 --identity* Use the sendemail. options.
 --to-cmd  * Email To: via ` \$patch_path`
 --cc-cmd  * Email Cc: via ` \$patch_path`
---suppress-cc * author, self, sob, cc, cccmd, body, 
bodycc, all.
+--suppress-cc * author, self, sob, cc, cccmd, body, 
bodycc, bylines, all.
 --[no-]cc-cover* Email Cc: addresses in the cover letter.
 --[no-]to-cover* Email To: addresses in the cover letter.
 --[no-]signed-off-by-cc* Send to Signed-off-by: addresses. Default 
on.
@@ -431,13 +431,13 @@ my(%suppress_cc);
 if (@suppress_cc) {
foreach my $entry (@suppress_cc) {
die "Unknown --suppress-cc field: '$entry'\n"
-   unless $entry =~ 
/^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/;
+   unless $entry =~ 
/^(?:all|cccmd|cc|author|self|sob|body|bodycc|bylines)$/;
$suppress_cc{$entry} = 1;
}
 }
 
 if ($suppress_cc{'all'}) {
-   foreach my $entry (qw (cccmd cc author self sob body bodycc)) {
+   foreach my $entry (qw (cccmd cc author self sob body bodycc bylines)) {
$suppress_cc{$entry} = 1;
}
delete $suppress_cc{'all'};
@@ -448,7 +448,7 @@ $suppress_cc{'self'} = $suppress_from if defined 
$suppress_from;
 $suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc;
 
 if ($suppress_cc{'body'}) {
-   foreach my $entry (qw (sob bodycc)) {
+   foreach my $entry (qw (sob bodycc bylines)) {
$suppress_cc{$entry} = 1;
}
delete $suppress_cc{'body'};
@@ -1545,7 +1545,7 @@ foreach my $t (@files) {
# Now parse the message body
while(<$fh>) {
$message .=  $_;
-   if (/^(Signed-off-by|Cc): (.*)$/i) {
+   if (/^(Signed-off-by|Cc|[^\s]+[\w-]by): (.*)$/i) {
chomp;
my ($what, $c) = ($1, $2);
chomp $c;
@@ -1555,6 +1555,12 @@ foreach my $t (@files) {
} else {
next if $suppress_cc{'sob'} and $what =~ 
/Signed-off-by/i;
next if $suppress_cc{'bodycc'} and $what =~ 
/Cc/i;
+   next if $suppress_cc{'bylines'} and $what !~ 
/Signed-off-by/i and $what =~ /by$/i;
+   }
+   if ($c !~ /.+@.+/) {
+   printf("(body) Ignoring %s from line '%s'\n",
+  $what, $_) unless $quiet;
+   next;
}
push @cc, $c;
printf("(body) Adding cc: %s from line '%s'\n",
-- 
2.10.0.rc2.1.gaa4c9e0

Re: [PATCH 08/22] sequencer: remove overzealous assumption

[Jakub Narębski]

Hello Johannes,

W dniu 31.08.2016 o 20:36, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Jakub Narębski wrote: 
>> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
 
> I am of the opinion that overzealous checking of certain implementation
> details is something to be avoided.

I agree.

>>> Therefore let's just get rid of the test that wants to verify that this
>>> limitation is still in place, in preparation for the upcoming work to
>>> teach the sequencer to do rebase -i's work.
>>
>> Is it "upcoming work" as in one of the patches in this series?
>> If so, which patch?
> 
> I left this a little vague, didn't I? ;-)
> 
> The problem is that the `git-rebase-todo` most definitely does *not* want
> to be restricted to a single command.
> 
> So if you must have a patch that disagrees with this overzealous check,
> the "revamp todo parsing" one is probably the first. But it is better to
> think of this at a higher level than just patches: it is wrong to limit
> the todo script to contain only identical commands.

I see.  Right.

I wonder: would 'git cherry-pick --continue' be able to finish
'git revert', and vice versa, then?  Or 'git sequencer --continue'?

>>> diff --git a/t/t3510-cherry-pick-sequence.sh 
>>> b/t/t3510-cherry-pick-sequence.sh
>>> index 7b7a89d..6465edf 100755
>>> --- a/t/t3510-cherry-pick-sequence.sh
>>> +++ b/t/t3510-cherry-pick-sequence.sh
>>> @@ -459,17 +459,6 @@ test_expect_success 'malformed instruction sheet 1' '
>>> test_expect_code 128 git cherry-pick --continue
>>>  '
>>>  
>>> -test_expect_success 'malformed instruction sheet 2' '
>>
>> Hmmm... the description is somewhat lacking (especially compared to
>> the rest of test), anyway.
>>
>> BTW. we should probably rename 'malformed instruction sheet 2'
>> to 'malformed instruction sheet' if there are no further such
>> tests after this removal, isn't it?
> 
> No, we cannot rename it after this patch because the patch removes it ;-)
> (It is not a file name but really a label for a test case.)

Ooops.  What I wanted to say that after removing the test case named
'malformed instruction sheet 2' we should also rename *earlier* test
case from 'malformed instruction sheet 1' to 'malformed instruction sheet',
as it is now the only 'malformed instruction sheet *' test case.

Re: git submodules implementation question

[Junio C Hamano]

Uma Srinivasan  writes:

> diff --git a/submodule.c b/submodule.c
> index 5a62aa2..23443a7 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -960,6 +960,9 @@ unsigned is_submodule_modified(const char *path,
> int ignore_untracked)
> return 0;
>
> }
> +   /* stuff submodule git dir into env var */
> +   set_git_dir(git_dir);

We want to affect only the process we are going to spawn to work
inside the submodule, not ourselves, which is what this call does;
this does not sound like a good idea.

Re: git submodules implementation question

[Uma Srinivasan]

FWIW, I tried the following quick change based on the info. provided
by Junio and it seems to "fix" my original problem. I'll let you
experts figure out if we need a more complete fix or not.

diff --git a/submodule.c b/submodule.c
index 5a62aa2..23443a7 100644
--- a/submodule.c
+++ b/submodule.c
@@ -960,6 +960,9 @@ unsigned is_submodule_modified(const char *path,
int ignore_untracked)
return 0;

}
+   /* stuff submodule git dir into env var */
+   set_git_dir(git_dir);
+
strbuf_reset();

if (ignore_untracked)
@@ -1279,5 +1282,9 @@ void prepare_submodule_repo_env(struct argv_array *out)
for (var = local_repo_env; *var; var++) {
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
+   if (strcmp(*var, GIT_DIR_ENVIRONMENT))
+   argv_array_pushf(out, "%s=%s", GIT_DIR_ENVIRONMENT,
+get_git_dir());
}
+
 }

Thanks,
Uma

On Wed, Aug 31, 2016 at 9:42 AM, Junio C Hamano  wrote:
> Uma Srinivasan  writes:
>
>>> I might suggest to update prepare_submodule_repo_env() so that the
>>> spawned process will *NOT* have to guess where the working tree and
>>> repository by exporting GIT_DIR (set to "git_dir" we discover above)
>>> and GIT_WORK_TREE (set to "." as cp.dir is set to the path to the
>>> working tree of the submodule).  That would stop the "git status" to
>>> guess (and fooled by a corrupted dir/.git that is not a git
>>> repository).
>>
>> Here's where I am struggling with my lack of knowledge of git
>> internals and the implementation particularly in the context of how
>> environment variables are passed from the parent to the child process.
>
> Ah, I was primarily addressing Jacob in the latter part of my
> message, as he's looked at similar things in his recent topic.
>
>> Are you suggesting that we set up the child process environment array
>> (the "out" argument) in prepare_submodule_repo_env() to include
>> GIT_DIR_ENVIRONMENT and GIT_WORK_TREE_ENVIRONMENT in addition to
>> CONFIG_DATA_ENVIRONMENT that is there now?
>
> I was wondering if we should unconditionally stuff GIT_DIR= repository location for the submodule> in the cp.env_array passed to
> the function prepare_submodule_repo_env().  As cp.dir will be set to
> the submodule's working tree, there is no need to set GIT_WORK_TREE
> and export it, I think, although it would not hurt.
>
> After all, we _are_ going into a separate and different project's
> repository (that is what a submodule is), and we _know_ where its
> repository data (i.e. GIT_DIR) and the location of its working tree
> (i.e. GIT_WORK_TREE).  There is no reason for the process that will
> work in the submodule to go through the usual "do we have .git in
> our $cwd that is a repository?  if not how about the parent directory
> of $cwd?  go up until we find one and that directory is the top of
> the working tree" discovery.
>
> More importantly, this matters when your GIT_DIR for the submodule
> is somehow corrupt.  The discovery process would say "there is .git
> in $cwd but that is not a repository" and continue upwards, which
> likely would find the repository for the top-level superproject,
> which definitely is _not_ want you want to happen.  And one way to
> avoid it is to tell the setup.c code that it should not do the
> discovery by being explicit.
>

Re: [PATCH 06/34] sequencer (rebase -i): write the 'done' file

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:54 +0200, Johannes Schindelin wrote:
> In the interactive rebase, commands that were successfully processed are
> not simply discarded, but appended to the 'done' file instead. This is
> used e.g. to display the current state to the user in the output of
> `git status` or the progress.

Wouldn't it make more sense to have this patch before the ones that
implement the actual rebase commands?

Hmm, and after reading more of this series, I think the same applies to
some other patches too, e.g. 08/34 and 14/34, so I'm probably missing
something. So before I make a fool of myself and suggest that the
implementation of the actual commands should come at the end, maybe you
could tell me what I'm missing :) 

D.

Re: [PATCH 15/34] sequencer (rebase -i): leave a patch upon error

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:55 +0200, Johannes Schindelin wrote:
> Just like the interactive rebase, we want to leave a 'patch' file for
> further inspection by the user (even if we never tried to actually apply
> that patch, since we're cherry-picking instead).
> 
> Signed-off-by: Johannes Schindelin 

This commit message confuses me. Did you mean s/Just like the/When
doing an/? 

D.

Re: [PATCH 00/34] Teach the sequencer to act as rebase -i's backend

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:53 +0200, Johannes Schindelin wrote:
> This marks the count down to '3': two more patch series after this
> (really tiny ones) and we have a faster rebase -i.

I got to 16/34 (and skipped 07/34), will continue tomorrow. I hope the
comments are useful.

D.

Re: [PATCH 05/34] sequencer (rebase -i): learn about the 'verbose' mode

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:54 +0200, Johannes Schindelin wrote:

> +   if (file_exists(rebase_path_verbose()))
> +   opts->verbose = 1;

I don't see anything in this series that creates this file, will that
be part of a later series?

D.

Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

[Jakub Narębski]

W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:

> The `git-rebase-todo` file contains a list of commands. Most of those
> commands have the form
> 
> 
> 
> The  is displayed primarily for the user's convenience, as
> rebase -i really interprets only the   part. However, there
> are *some* places in interactive rebase where the  is used to
> display messages, e.g. for reporting at which commit we stopped.
> 
> So let's just remember it when parsing the todo file; we keep a copy of
> the entire todo file anyway (to write out the new `done` and
> `git-rebase-todo` file just before processing each command), so all we
> need to do is remember the begin and end offsets.

Actually what we remember is pointer and length, or begin offset and length,
not offset and offset.

> 
> Signed-off-by: Johannes Schindelin 

Nice, I'll see how it is used later (and in which commit in series).

> ---
>  sequencer.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 06759d4..3398774 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts 
> *opts)
>  struct todo_item {
>   enum todo_command command;
>   struct commit *commit;
> + const char *arg;
> + int arg_len;

Why 'arg', and not 'oneline', or 'subject'?
I'm not saying it is bad name.

>   size_t offset_in_buf;
>  };
>  
> @@ -760,6 +762,9 @@ static int parse_insn_line(struct todo_item *item, const 
> char *bol, char *eol)
>   status = get_sha1(bol, commit_sha1);
>   *end_of_object_name = saved;
>  
> + item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> + item->arg_len = (int)(eol - item->arg);
> +

Does it work correctly for line without , that is

 

I think it does, but I not entirely sure.

>   if (status < 0)
>   return -1;
>  
> @@ -880,6 +885,8 @@ static int walk_revs_populate_todo(struct todo_list 
> *todo_list,
>  
>   item->command = command;
>   item->commit = commit;
> + item->arg = NULL;
> + item->arg_len = 0;
>   item->offset_in_buf = todo_list->buf.len;
>   subject_len = find_commit_subject(commit_buffer, );
>   strbuf_addf(_list->buf, "%s %s %.*s\n",
> 

Re: [PATCH 08/22] sequencer: remove overzealous assumption

[Johannes Schindelin]

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> > The sequencer was introduced to make the cherry-pick and revert
> > functionality available as library function, with the original idea
> > being to extend the sequencer to also implement the rebase -i
> > functionality.
> > 
> > The test to ensure that all of the commands in the script are identical
> > to the overall operation does not mesh well with that.
> 
> Actually the question is what does the test that got removed in this
> commit actually check.  Is it high-level sanity check that todo list
> for git-cherry-pick contains only 'pick', and for git-revert contains
> only 'revert'?

It might have been that at some stage.

But should we really check that? Or should we check the *effects*?

I am of the opinion that overzealous checking of certain implementation
details is something to be avoided.

> > Therefore let's just get rid of the test that wants to verify that this
> > limitation is still in place, in preparation for the upcoming work to
> > teach the sequencer to do rebase -i's work.
> 
> Is it "upcoming work" as in one of the patches in this series?
> If so, which patch?

I left this a little vague, didn't I? ;-)

The problem is that the `git-rebase-todo` most definitely does *not* want
to be restricted to a single command.

So if you must have a patch that disagrees with this overzealous check,
the "revamp todo parsing" one is probably the first. But it is better to
think of this at a higher level than just patches: it is wrong to limit
the todo script to contain only identical commands.

> > diff --git a/t/t3510-cherry-pick-sequence.sh 
> > b/t/t3510-cherry-pick-sequence.sh
> > index 7b7a89d..6465edf 100755
> > --- a/t/t3510-cherry-pick-sequence.sh
> > +++ b/t/t3510-cherry-pick-sequence.sh
> > @@ -459,17 +459,6 @@ test_expect_success 'malformed instruction sheet 1' '
> > test_expect_code 128 git cherry-pick --continue
> >  '
> >  
> > -test_expect_success 'malformed instruction sheet 2' '
> 
> Hmmm... the description is somewhat lacking (especially compared to
> the rest of test), anyway.
> 
> BTW. we should probably rename 'malformed instruction sheet 2'
> to 'malformed instruction sheet' if there are no further such
> tests after this removal, isn't it?

No, we cannot rename it after this patch because the patch removes it ;-)
(It is not a file name but really a label for a test case.)

Thanks for the review,
Johannes

Re: [PATCH 11/22] sequencer: get rid of the subcommand field

[Jakub Narębski]

W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:

> The subcommands are used exactly once, at the very beginning of
> sequencer_pick_revisions(), to determine what to do. This is an
> unnecessary level of indirection: we can simply call the correct
> function to begin with. So let's do that.

Looks good.  Parsing is moved from parse_args(), now unnecessary,
to the new run_sequencer().  Which also picked up dispatch from
sequencer_pick_revisions() - that sometimes didn't pick revisions :-o.

"All problems in computer science can be solved by another level
 of indirection, except of course for the problem of too many
 indirections." -- David John Wheeler

> 
> While at it, ensure that the subcommands return an error code so that
> they do not have to die() all over the place (bad practice for library
> functions...).

This perhaps should be moved to a separate patch, but I guess
there is a reason behind "while at it".

Also subcommand functions no longer are local to sequencer.c

> 
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/revert.c | 36 
>  sequencer.c  | 35 +++
>  sequencer.h  | 13 -
>  3 files changed, 31 insertions(+), 53 deletions(-)

Nice size reduction.

Re: [PATCH] git-send-email: Add ability to cc: any "trailers" from commit message

[Joe Perches]

On Wed, 2016-08-31 at 10:54 -0700, Junio C Hamano wrote:
> Joe Perches  writes:
> > 
> > Many commits have various forms of trailers similar to
> >  "Acked-by: Name " and "Reported-by: Name "
> > 
> > Add the ability to cc these trailers when using git send-email.
> I thought you were asking what we call these " followed by
> " at the end of the log message, and "footers or trailers"
> was the answer.
> 
> I do not have a strong objection against limiting to "-by:" lines;
> for one thing, it would automatically avoid having to worry about
> "Bug-ID:" and other trailers that won't have e-mail address at all.
> 
> But if you are _only_ picking up "-by:" lines, then calling this
> option "trailers" is way too wide and confusing.  I do not think
> there is any specific name for "-by:" lines, though.  Perhaps you
> would need to invent some name that has "-by" as a substring.
> 
> "any-by"?  or just "by"?  I dunno.

Thinking about this a little, "bylines" seems much better.

> >@@ -1545,7 +1545,7 @@ foreach my $t (@files) {
> >     # Now parse the message body
> >     while(<$fh>) {
> >     $message .=  $_;
> > -   if (/^(Signed-off-by|Cc): (.*)$/i) {
> > +   if (/^(Signed-off-by|Cc|[^\s]+[_-]by): (.*)$/i) {
> Micronits:
> 
>  (1) do you really want to grab a run of any non-blanks?  Don't
>  you want to exclude at least a colon?

It could use [\w_-]+

>  (2) allowing an underscore looks a bit unusual.  

It's for typos.  A relatively high percentage of
these things in at least the kernel were malformed
when I started this 5 years ago.

I don't have an objection to requiring the proper
form using only dashes though.

Maybe that'd help reduce the typo frequency anyway.

> I am aware of the fact that people sometimes write only a name with
> no e-mail address when giving credit to a third-party and we want to
> avoid upsetting the underlying MTA by feeding it a non-address.
> 
> Looking at existing helper subs like extract_valid_address and
> sanitize_address that all addresses we pass to the MTA go through,
> it appears to me that we try to support an addr-spec with only
> local-part without @domain, so this new check might turn out to be
> too strict from that point of view, but on the other hand I suspect
> it won't be a huge issue because the addresses in the footers are
> for public consumption and it may not make much sense to have a
> local-only address there.  I dunno.
> 
> > 
> >     push @cc, $c;
> >     printf("(body) Adding cc: %s from line '%s'\n",

me either but I think it doesn't hurt because
as you suggest, these are supposed to be public.

Thanks for the review.

cheers, Joe

Re: [PATCH] git-send-email: Add ability to cc: any "trailers" from commit message

[Junio C Hamano]

Joe Perches  writes:

> On Wed, 2016-08-31 at 10:54 -0700, Junio C Hamano wrote:
>> Joe Perches  writes:
>> 
>> > 
>> > Many commits have various forms of trailers similar to
>> >  "Acked-by: Name " and "Reported-by: Name "
>> > 
>> > Add the ability to cc these trailers when using git send-email.
>> I thought you were asking what we call these " followed by
>> " at the end of the log message, and "footers or trailers"
>> was the answer.
>> 
>> I do not have a strong objection against limiting to "-by:" lines;
>> for one thing, it would automatically avoid having to worry about
>> "Bug-ID:" and other trailers that won't have e-mail address at all.
>> 
>> But if you are _only_ picking up "-by:" lines, then calling this
>> option "trailers" is way too wide and confusing.  I do not think
>> there is any specific name for "-by:" lines, though.  Perhaps you
>> would need to invent some name that has "-by" as a substring.
>> 
>> "any-by"?  or just "by"?  I dunno.
>
> Signatures?

Helped-by: and Reported-by: are often written by the author of the
patch and not by those who helped or reported, so calling them signatures
imply the author is forging them, too ;-)

I dunno.

Any name that hints that this applies only to the trailers that ends
with "-by" is fine but "Signatures-by" does not sound very
grammatical.

Re: [PATCH 10/22] sequencer: avoid completely different messages for different actions

[Jakub Narębski]

CC-ed to Jiang Xin, L10N coordinator.

W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:

> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index cbdce6d..1b65202 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -232,11 +232,8 @@ static int error_dirty_index(struct replay_opts *opts)
>   if (read_cache_unmerged())
>   return error_resolve_conflict(action_name(opts));
>  
> - /* Different translation strings for cherry-pick and revert */
> - if (opts->action == REPLAY_PICK)
> - error(_("Your local changes would be overwritten by 
> cherry-pick."));
> - else
> - error(_("Your local changes would be overwritten by revert."));
> + error(_("Your local changes would be overwritten by %s."),
> + action_name(opts));

If I understand it correctly, it would make "revert" or "cherry-pick"
untranslated part of error message.  You would need to use translation
on the result with "_(action_name(opts))", you would have to mark
todo_command_strings elements for gettext lexicon with N_(...).

I am rather against this change (see also below).


>From the first glance I though that there would be no problem with
translation legos / jigsaw it introduces, namely that the "revert"
and "cherry-pick" would require different rest of text:

 po/bg.po:msgid "Your local changes would be overwritten by cherry-pick."
 po/bg.po-msgstr "Локалните ви промени ще бъдат презаписани при отбирането на 
подавания."
 --
 po/bg.po:msgid "Your local changes would be overwritten by revert."
 po/bg.po-msgstr "Локалните ви промени ще бъдат презаписани при отмяната на 
подавания."

 po/de.po:msgid "Your local changes would be overwritten by cherry-pick."
 po/de.po-msgstr "Ihre lokalen Änderungen würden durch den Cherry-Pick 
überschrieben werden."
 --
 po/de.po:msgid "Your local changes would be overwritten by revert."
 po/de.po-msgstr "Ihre lokalen Änderungen würden durch den Revert überschrieben 
werden."

But it turns out that "revert" and "cherry-pick" can be of different
gender:

 po/ca.po:msgid "Your local changes would be overwritten by cherry-pick."
 po/ca.po-msgstr "Els vostres canvis locals se sobreescriurien pel recull de 
cireres."
 --
 po/ca.po:msgid "Your local changes would be overwritten by revert."
 po/ca.po-msgstr "Els vostres canvis locals se sobreescriurien per la reversió."

In some cases "revert" and "cherry-pick" are not translated literally
(but compare translation for similar language: po/bg.po, without this):

 po/ru.po:msgid "Your local changes would be overwritten by cherry-pick."
 po/ru.po-msgstr "Ваши локальные изменение будут перезаписаны отбором лучшего."
 --
 po/ru.po:msgid "Your local changes would be overwritten by revert."
 po/ru.po-msgstr "Ваши локальные изменение будут перезаписаны возвратом 
коммита."

Similar for (but here one side uses untranslated English term...):

 po/vi.po:msgid "Your local changes would be overwritten by cherry-pick."
 po/vi.po-msgstr "Các thay đổi nội bộ của bạn có thể bị ghi đè bởi lệnh 
cherry-pick."
 --
 po/vi.po:msgid "Your local changes would be overwritten by revert."
 po/vi.po-msgstr "Các thay đổi nội bộ của bạn có thể bị ghi đè bởi lệnh hoàn 
nguyên."

For some I don't know which is the case:

 po/zh_CN.po:msgid "Your local changes would be overwritten by cherry-pick."
 po/zh_CN.po-msgstr "您的本地修改将被拣选操作覆盖。"
 --
 po/zh_CN.po:msgid "Your local changes would be overwritten by revert."
 po/zh_CN.po-msgstr "您的本地修改将被还原操作覆盖。"

Unless we want to require to use English terms:

 po/sv.po:msgid "Your local changes would be overwritten by cherry-pick."
 po/sv.po-msgstr "Dina lokala ändringar skulle skrivas över av \"cherry-pick\"."
 --
 po/sv.po:msgid "Your local changes would be overwritten by revert."
 po/sv.po-msgstr "Dina lokala ändringar skulle skrivas över av \"revert\"."


>  
>   if (advice_commit_before_merge)
>   advise(_("Commit your changes or stash them to proceed."));
> 

Re: [PATCH] git-send-email: Add ability to cc: any "trailers" from commit message

[Joe Perches]

On Wed, 2016-08-31 at 10:54 -0700, Junio C Hamano wrote:
> Joe Perches  writes:
> 
> > 
> > Many commits have various forms of trailers similar to
> >  "Acked-by: Name " and "Reported-by: Name "
> > 
> > Add the ability to cc these trailers when using git send-email.
> I thought you were asking what we call these " followed by
> " at the end of the log message, and "footers or trailers"
> was the answer.
> 
> I do not have a strong objection against limiting to "-by:" lines;
> for one thing, it would automatically avoid having to worry about
> "Bug-ID:" and other trailers that won't have e-mail address at all.
> 
> But if you are _only_ picking up "-by:" lines, then calling this
> option "trailers" is way too wide and confusing.  I do not think
> there is any specific name for "-by:" lines, though.  Perhaps you
> would need to invent some name that has "-by" as a substring.
> 
> "any-by"?  or just "by"?  I dunno.

Signatures?

Re: [PATCH] git-send-email: Add ability to cc: any "trailers" from commit message

[Junio C Hamano]

Joe Perches  writes:

> Many commits have various forms of trailers similar to
>  "Acked-by: Name " and "Reported-by: Name "
>
> Add the ability to cc these trailers when using git send-email.

I thought you were asking what we call these " followed by
" at the end of the log message, and "footers or trailers"
was the answer.

I do not have a strong objection against limiting to "-by:" lines;
for one thing, it would automatically avoid having to worry about
"Bug-ID:" and other trailers that won't have e-mail address at all.

But if you are _only_ picking up "-by:" lines, then calling this
option "trailers" is way too wide and confusing.  I do not think
there is any specific name for "-by:" lines, though.  Perhaps you
would need to invent some name that has "-by" as a substring.

"any-by"?  or just "by"?  I dunno.

>  if ($suppress_cc{'all'}) {
> - foreach my $entry (qw (cccmd cc author self sob body bodycc)) {
> + foreach my $entry (qw (cccmd cc author self sob body bodycc trailers)) {
>   $suppress_cc{$entry} = 1;
>   }

OK.

> @@ -448,7 +448,7 @@ $suppress_cc{'self'} = $suppress_from if defined 
> $suppress_from;
>  $suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc;
>  
>  if ($suppress_cc{'body'}) {
> - foreach my $entry (qw (sob bodycc)) {
> + foreach my $entry (qw (sob bodycc trailers)) {
>   $suppress_cc{$entry} = 1;
>   }
>   delete $suppress_cc{'body'};

OK.

> @@ -1545,7 +1545,7 @@ foreach my $t (@files) {
>   # Now parse the message body
>   while(<$fh>) {
>   $message .=  $_;
> - if (/^(Signed-off-by|Cc): (.*)$/i) {
> + if (/^(Signed-off-by|Cc|[^\s]+[_-]by): (.*)$/i) {

Micronits:

 (1) do you really want to grab a run of any non-blanks?  Don't
 you want to exclude at least a colon?
 (2) allowing an underscore looks a bit unusual.  

> @@ -1555,6 +1555,12 @@ foreach my $t (@files) {
>   } else {
>   next if $suppress_cc{'sob'} and $what =~ 
> /Signed-off-by/i;
>   next if $suppress_cc{'bodycc'} and $what =~ 
> /Cc/i;
> + next if $suppress_cc{'trailers'} and $what !~ 
> /Signed-off-by/i && $what =~ /by$/i;
> + }

It is a bit unfortunate that S-o-b is a subset of any-by that forces
you to do this.

> + if ($c !~ /.+@.+/) {
> + printf("(body) Ignoring %s from line '%s'\n",
> +$what, $_) unless $quiet;
> + next;
>   }

This check is new and applies to sob/cc, too.

I am aware of the fact that people sometimes write only a name with
no e-mail address when giving credit to a third-party and we want to
avoid upsetting the underlying MTA by feeding it a non-address.

Looking at existing helper subs like extract_valid_address and
sanitize_address that all addresses we pass to the MTA go through,
it appears to me that we try to support an addr-spec with only
local-part without @domain, so this new check might turn out to be
too strict from that point of view, but on the other hand I suspect
it won't be a huge issue because the addresses in the footers are
for public consumption and it may not make much sense to have a
local-only address there.  I dunno.

>   push @cc, $c;
>   printf("(body) Adding cc: %s from line '%s'\n",

Re: [PATCH 09/22] sequencer: completely revamp the "todo" script parsing

[Jakub Narębski]

W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:

> When we came up with the "sequencer" idea, we really wanted to have
> kind of a plumbing equivalent of the interactive rebase. Hence the
> choice of words: the "todo" script, a "pick", etc.
> 
> However, when it came time to implement the entire shebang, somehow this
> idea got lost and the sequencer was used as working horse for
> cherry-pick and revert instead. So as not to interfere with the
> interactive rebase, it even uses a separate directory to store its
> state.
> 
> Furthermore, it also is stupidly strict about the "todo" script it
> accepts: while it parses commands in a way that was *designed* to be
> similar to the interactive rebase, it then goes on to *error out* if the
> commands disagree with the overall action (cherry-pick or revert).

Does this mean that after the change you would be able to continue
"git revert" with "git cherry-pick --continue", and vice versa?  Or that
it would be possible for git-cherry-pick to do reverts (e.g. with ^)?

That's what we need to decide before becoming more lenient.
 
BTW. perhaps we would be able to continue with 'git continue', regardless
of what we have started with, I wonder...

>
> Finally, the sequencer code chose to deviate from the interactive rebase
> code insofar that it *reformats* the "todo" script instead of just
> writing the part of the parsed script that were not yet processed. This
> is not only unnecessary churn, but might well lose information that is
> valuable to the user (i.e. comments after the commands).

That's a very good change.

>
> Let's just bite the bullet and rewrite the entire parser; the code now
> becomes not only more elegant: it allows us to go on and teach the
> sequencer how to parse *true* "todo" scripts as used by the interactive
> rebase itself. In a way, the sequencer is about to grow up to do its
> older brother's job. Better.

Sidenote: this is not your fault, but Git doesn't do a good job on
changes which are mostly rewrites, trying to match stray '}' and the
like in generated diff.  I wonder if existing diff heuristic options
could help here.

> 
> While at it, do not stop at the first problem, but list *all* of the
> problems. This helps the user by allowing to address all issues in
> one go rather than going back and forth until the todo list is valid.

That is also a good change, though I wonder how often users need
to worry about this outside interactive rebase case.  If it is
preparation for rebase -i, where instruction list is written by
prone to errors human, it would be nice to have this information
in the commit message.

> 
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 241 
> +---
>  1 file changed, 134 insertions(+), 107 deletions(-)

Note: I have moved some lines of diff so that the change is more
readable to humans (but it results often in --++-++ chunk).

> 
> diff --git a/sequencer.c b/sequencer.c
> index 982b6e9..cbdce6d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -473,7 +473,26 @@ static int allow_empty(struct replay_opts *opts, struct 
> commit *commit)
>   return 1;
>  }
>  
> -static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> +enum todo_command {
> + TODO_PICK,
> + TODO_REVERT
> +};

Do we have a naming convention for enums elements?  Or are we explicitly
making enums and #defines interchangeable?  I wonder...

...uh, I see we don't have naming convention, but all caps snake-case
names dominate:

  $ git grep -A2 'enum .* {'
  [...]
  diff.h:enum color_diff {
  diff.h- DIFF_RESET = 0,
  diff.h- DIFF_CONTEXT = 1,
  --
  dir.c:enum path_treatment {
  dir.c-  path_none = 0,
  dir.c-  path_recurse,
  --

Shouldn't we say 'TODO_PICK = 0' explicitly, though?

> +
> +static const char *todo_command_strings[] = {
> + "pick",
> + "revert"
> +};

It's a bit pity that we cannot use designated inits, and hanging comma,
(from ISO C99 standard).  That is:

  +static const char *todo_command_strings[] = {
  + [TODO_PICK]   = "pick",
  + [TODO_REVERT] = "revert",
  +};


> +
> +static const char *command_to_string(const enum todo_command command)
> +{
> + if (command < ARRAY_SIZE(todo_command_strings))
> + return todo_command_strings[command];
> + die("Unknown command: %d", command);
> +}
> +
> +
> +static int do_pick_commit(enum todo_command command, struct commit *commit,
> + struct replay_opts *opts)
>  {
>   unsigned char head[20];
>   struct commit *base, *next, *parent;
> @@ -535,7 +554,8 @@ static int do_pick_commit(struct commit *commit, struct 
> replay_opts *opts)
>   /* TRANSLATORS: The first %s will be "revert" or
>  "cherry-pick", the second %s a SHA1 */
>   return error(_("%s: cannot parse parent commit %s"),

I wonder if we should not change also the error message: it is no
longer about 

Re: [PATCH v2 00/11] Mark strings in perl script for translation

[Junio C Hamano]

Vasco Almeida  writes:

> Mark messages in some perl scripts for translation.
>
> Since v1, adds brackets so parameter grouping of sprintf parameters is easier
> to see.
>
> Interdiff included below.

Thanks; it is way too late for this cycle for i18n so I won't be
picking the series up right now.  Please ping me if you see me
forget to pick it up in a week after 2.10 final gets tagged.

Re: [GIT PULL] l10n updates for 2.10.0 round 2

[Junio C Hamano]

Jiang Xin  writes:

> Would you please pull the following git l10n updates.
>
> The following changes since commit d5cb9cbd64165153a318e1049f8bf14b09a16b11:
>
>   Git 2.10-rc2 (2016-08-26 13:59:20 -0700)
>
> are available in the git repository at:
>
>   git://github.com/git-l10n/git-po tags/l10n-2.10.0-rnd2
>
> for you to fetch changes up to 5c57d7622e3c7c685345f9736c4c924349a321dd:
>
>   l10n: zh_CN: for git v2.10.0 l10n round 2 (2016-08-31 00:11:13 +0800)

Thanks.  Pulled.

Request for advice: visual diffing kicad EDA schematics

[Jean-Noël AVILA]

Hi all,

I would like to be able to visually see the differences between two versions 
of schematic sheets of a kicad project.

Kicad file format is ascii based, which allows to generate text diff between 
versions of schematic files. However, except for adding and removing 
components, when the modified lines refer to drawing primitives, it isn't 
obvious what has changed.

Schematic file format differs from text picture formats (e.g. svg, pnm) in that 
the schematics is made of an assembly of lines, texts and references to 
electronic parts whose symbols are stored in external library files. Hopefully, 
for each project, Kicad stores a local cache library of all the parts used in 
the project, so the project directory is self contained.

But of course, to be able to plot a schematic sheet, you need the schematic 
sheet file and the project cache library at the same version, which defeats the 
custom diff mechanics of git that can be used for plain pictures.

My questions:
 * Does this sound familiar to any body ?
 * Is there some hook in git to deal with this or will I have to roll my own 
utility? In which case, would you advise to use git porcelain or libgit binded 
to a favorite script language?

Thanks in advance for all information.

Jean-Noël

Re: [PATCH v11 0/8] submodule inline diff format

[Junio C Hamano]

Jacob Keller  writes:

>> We probably should release for the error case. I'll do that. I don't
>> believe do_submodule_path ensures that the passed in argument is
>> guaranteed to not be initialized or used.
>> 
>> Thanks,
>> Jake
>
> Here's the squash for this fix.

It seems that the topic has been quiet since the message I am
responding to.  Have all the issues been resolved, or are there
still some loose ends to be hashed out?

Hoping that the answer is "the former", I'll mark this topic as
"Waiting for a reroll."

Thanks.

Re: git submodules implementation question

[Junio C Hamano]

Uma Srinivasan  writes:

>> I might suggest to update prepare_submodule_repo_env() so that the
>> spawned process will *NOT* have to guess where the working tree and
>> repository by exporting GIT_DIR (set to "git_dir" we discover above)
>> and GIT_WORK_TREE (set to "." as cp.dir is set to the path to the
>> working tree of the submodule).  That would stop the "git status" to
>> guess (and fooled by a corrupted dir/.git that is not a git
>> repository).
>
> Here's where I am struggling with my lack of knowledge of git
> internals and the implementation particularly in the context of how
> environment variables are passed from the parent to the child process.

Ah, I was primarily addressing Jacob in the latter part of my
message, as he's looked at similar things in his recent topic.

> Are you suggesting that we set up the child process environment array
> (the "out" argument) in prepare_submodule_repo_env() to include
> GIT_DIR_ENVIRONMENT and GIT_WORK_TREE_ENVIRONMENT in addition to
> CONFIG_DATA_ENVIRONMENT that is there now?

I was wondering if we should unconditionally stuff GIT_DIR= in the cp.env_array passed to
the function prepare_submodule_repo_env().  As cp.dir will be set to
the submodule's working tree, there is no need to set GIT_WORK_TREE
and export it, I think, although it would not hurt.

After all, we _are_ going into a separate and different project's
repository (that is what a submodule is), and we _know_ where its
repository data (i.e. GIT_DIR) and the location of its working tree
(i.e. GIT_WORK_TREE).  There is no reason for the process that will
work in the submodule to go through the usual "do we have .git in
our $cwd that is a repository?  if not how about the parent directory
of $cwd?  go up until we find one and that directory is the top of
the working tree" discovery.

More importantly, this matters when your GIT_DIR for the submodule
is somehow corrupt.  The discovery process would say "there is .git
in $cwd but that is not a repository" and continue upwards, which
likely would find the repository for the top-level superproject,
which definitely is _not_ want you want to happen.  And one way to
avoid it is to tell the setup.c code that it should not do the
discovery by being explicit.

Re: [PATCH v6 00/12] Update git revisions

[Junio C Hamano]

Philip Oakley  writes:

> These are the quick fixes to Marc's comments to patches 5,6,11,
> and a consequential change to 12.
>
> Only the changed patches are included.

I gave a cursory re-read on the whole thing again, and spotted no
further issue.  This has been kept in the "Undecided" pile, but I'll
mark it as "Will merge to 'next'" and hope to have it in 'master'
early in the post 2.10 cycle.

Thanks.

Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 06:48 -0400, Jeff King wrote:
> On Wed, Aug 31, 2016 at 11:12:26AM +0200, Dennis Kaarsemaker wrote:
> 
> > 
> > That is indeed a bug. git reads the config of t1 and then thinks a
> > template config has set that value, so it won't override it.
> This is a regression in v2.9.0 due to my ae5f677 (lazily load
> core.sharedrepository, 2016-03-11).
> 
> > 
> > This is caused by git init reading the config via
> > get_shared_repository. The comment above it indicates that this may
> > not
> > be needed, and indeed not doing it makes this bug go away.
> Hrm. I'm not sure if that will work, though, because we may call
> get_shared_repository() from other code-paths (e.g., anything that
> calls
> adjust_shared_perm(), like safe_create_leading_directories).

Agreed, the diff was more to point out what triggered this (so I could
send that and run away to spend the day with jr.) than an attempt at a
patch.

> We may need to do something like turn off the
> need_shared_repository_from_config in init-db, since I think it would
> not want to ever read from the default config sources in most of its
> code-paths (OTOH, it should in theory respect core.sharedRepository
> in ~/.gitconfig, so maybe there is another more elegant way of
> handling this).

I would go even further and say that git init should completely ignore
the config of a repository you happen to be in when creating a new
repository.

> I'm out of time for the day, so it will be a while before I can dig
> further. Please feel free to figure it out while I am sleeping. :)
> 
> -Peff

I hope you slept well :)

This is what I came up with to implement my suggestion above, comments
welcome.

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 3a45f0b..d0fd3dc 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -493,6 +493,12 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
    int mkdir_tried = 0;
    retry:
    if (chdir(argv[0]) < 0) {
+   /*
+    * We're creating a new repository. If we're already in 
another
+    * repository, ignore its config
+    */
+   ignore_repo_config = 1;
+   git_config_clear();
    if (!mkdir_tried) {
    int saved;
    /*
@@ -500,7 +506,6 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
     * and we know shared_repository should always 
be 0;
     * but just in case we play safe.
     */
-   saved = get_shared_repository();
    set_shared_repository(0);
    switch 
(safe_create_leading_directories_const(argv[0])) {
    case SCLD_OK:
@@ -513,7 +518,6 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
    die_errno(_("cannot mkdir %s"), 
argv[0]);
    break;
    }
-   set_shared_repository(saved);
    if (mkdir(argv[0], 0777) < 0)
    die_errno(_("cannot mkdir %s"), 
argv[0]);
    mkdir_tried = 1;
@@ -524,6 +528,11 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
    } else if (0 < argc) {
    usage(init_db_usage[0]);
    }
+
+   need_shared_repository_from_config = 1;
+   ignore_repo_config = 0;
+   git_config_clear();
+
    if (is_bare_repository_cfg == 1) {
    char *cwd = xgetcwd();
    setenv(GIT_DIR_ENVIRONMENT, cwd, argc > 0);
diff --git a/cache.h b/cache.h
index f30a441..1be16fc 100644
--- a/cache.h
+++ b/cache.h
@@ -640,6 +640,8 @@ extern int hold_locked_index(struct lock_file *, int);
 extern void set_alternate_index_output(const char *);
 
 /* Environment bits from configuration mechanism */
+extern int ignore_repo_config;
+extern int need_shared_repository_from_config;
 extern int trust_executable_bit;
 extern int trust_ctime;
 extern int check_stat;
diff --git a/config.c b/config.c
index 0dfed68..2df0189 100644
--- a/config.c
+++ b/config.c
@@ -1304,7 +1304,7 @@ static int do_git_config_sequence(config_fn_t fn, void 
*data)
    ret += git_config_from_file(fn, user_config, data);
 
    current_parsing_scope = CONFIG_SCOPE_REPO;
-   if (repo_config && !access_or_die(repo_config, R_OK, 0))
+   if (repo_config && !ignore_repo_config && !access_or_die(repo_config, 
R_OK, 0))
    ret += git_config_from_file(fn, repo_config, data);
 
    current_parsing_scope = CONFIG_SCOPE_CMDLINE;
diff --git a/environment.c b/environment.c
index ca72464..f6dbba8 100644
--- a/environment.c
+++ b/environment.c

Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Johannes Schindelin]

Hi Ævar,

On Wed, 31 Aug 2016, Ævar Arnfjörð Bjarmason wrote:

> I haven't used it myself (or any Windows thing) but people say good
> things about http://strawberryperl.com

Ah yes. This comes up frequently. Many a Git for Windows user pointed me
into that direction.

The biggest problem with Strawberry Perl is that it is virtually
impossible to build the Subversion-Perl bindings using the Git for Windows
SDK when using Strawberry Perl.

Which pretty much precludes it from being used in Git for Windows.

And then there are the path issues... Git's Perl scripts are pretty
certain that they live in a POSIX-y environment. Which MSYS2 Perl
provides. Strawberry Perl not.

Ciao,
Johannes

git-completion.zsh does not escape spaces in filenames

[Sebastian Boehm]

Hi,

git-completion.zsh does not seem to properly escape spaces in filenames.

My git version: 2.9.2
My zsh version: 5.0.8

Expected behaviour: When completing filenames that contain spaces, for
example "foo bar", I would expect the git completion to complete "git
add foo" to "git add foo\ bar".

Actual behaviour:

touch "foo bar"
git add foo

results in:
git add foo bar

I am not a zsh expert, but apparently the usage of "compadd -Q" in
contrib/complete/git-completion.zsh prevents zsh from escaping spaces.

Is this behaviour intentional, or is this a bug?

Best,
Sebastian

[GIT PULL] l10n updates for 2.10.0 round 2

[Jiang Xin]

Hi Junio,

Would you please pull the following git l10n updates.

The following changes since commit d5cb9cbd64165153a318e1049f8bf14b09a16b11:

  Git 2.10-rc2 (2016-08-26 13:59:20 -0700)

are available in the git repository at:

  git://github.com/git-l10n/git-po tags/l10n-2.10.0-rnd2

for you to fetch changes up to 5c57d7622e3c7c685345f9736c4c924349a321dd:

  l10n: zh_CN: for git v2.10.0 l10n round 2 (2016-08-31 00:11:13 +0800)


l10n-2.10.0-rnd2


Alex Henrie (1):
  l10n: ca.po: update translation

Changwoo Ryu (1):
  l10n: ko.po: Update Korean translation

Jean-Noel Avila (1):
  l10n: fr.po v2.10.0-rc2

Jiang Xin (5):
  l10n: git.pot: v2.10.0 round 1 (248 new, 56 removed)
  Merge branch 'master' of git://github.com/git-l10n/git-po
  l10n: git.pot: v2.10.0 round 2 (12 new, 44 removed)
  Merge branch 'master' of https://github.com/vnwildman/git
  l10n: zh_CN: for git v2.10.0 l10n round 2

Peter Krefting (1):
  l10n: sv.po: Update Swedish translation (2757t0f0u)

Trần Ngọc Quân (1):
  l10n: Updated Vietnamese translation for v2.10.0 (2789t)

Vasco Almeida (2):
  l10n: pt_PT: merge git.pot
  l10n: pt_PT: update Portuguese translation

 po/ca.po| 6083 +--
 po/fr.po| 4868 +--
 po/git.pot  | 4325 +-
 po/ko.po| 4665 -
 po/pt_PT.po | 4975 ++--
 po/sv.po| 4834 +--
 po/vi.po| 4875 +--
 po/zh_CN.po | 4515 +++-
 8 files changed, 24393 insertions(+), 14747 deletions(-)

--
Jiang Xin

Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 31, 2016 at 12:29 PM, Johannes Schindelin
 wrote:
> Hi Ævar,
>
> On Tue, 30 Aug 2016, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, Aug 30, 2016 at 10:51 PM, Jeff King  wrote:
>> > On Tue, Aug 30, 2016 at 10:48:19PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> >  -failed: $(patsubst trash,,$(patsubst directory.%,%.sh,$(wildcard 
>> >> > trash\ directory.t[0-9]*)))
>> >> >  +failed:
>> >> >  +  @failed=$$(cd '$(TEST_RESULTS_DIRECTORY_SQ)' && \
>> >> >  +  grep -l '^failed [1-9]' $$(ls -t *.counts | \
>> >> >  +  sed 
>> >> > 'G;h;/^\(t[^.]*\)-[0-9]*\..*\n\1-[0-9]*\./d;P;d') | \
>> >> >  +  sed -n 's/-[0-9]*\.counts$$/.sh/p') && \
>> >> >  +  test -z "$$failed" || $(MAKE) $$failed
>> >> >
>> >> >   prove: pre-clean $(TEST_LINT)
>> >> > @echo "*** prove ***"; $(PROVE) --exec '$(SHELL_PATH_SQ)' 
>> >> > $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
>> >>
>> >> I don't at all mind this solution to the problem, if it works for that's 
>> >> cool.
>> >>
>> >> But FWIW something you may have missed is that you can just use
>> >> prove(1) for this, which is why I initially patched git.git to support
>> >> TAP, so I didn't have to implement stuff like this.
>> >
>> > Heh. I think each iteration of this patch will be destined to have
>> > somebody[1] point Johannes at prove. ;)
>> >
>> > (But I really do recommend prove if you can use it).
>> >
>> > -Peff
>> >
>> > [1] 
>> > http://public-inbox.org/git/20160630063725.gc15...@sigill.intra.peff.net/
>>
>> Sorry about that, I see it's been mentioned already.
>
> Yeah, it is true that prove(1) would be able to help. If it worked
> reliably on Windows. (Probably Perl's fault, not prove's.)

I haven't used it myself (or any Windows thing) but people say good
things about http://strawberryperl.com

Re: [PATCH 08/22] sequencer: remove overzealous assumption

[Jakub Narębski]

W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> The sequencer was introduced to make the cherry-pick and revert
> functionality available as library function, with the original idea
> being to extend the sequencer to also implement the rebase -i
> functionality.
> 
> The test to ensure that all of the commands in the script are identical
> to the overall operation does not mesh well with that.

Actually the question is what does the test that got removed in this
commit actually check.  Is it high-level sanity check that todo list
for git-cherry-pick contains only 'pick', and for git-revert contains
only 'revert'?  Or does it check that at the low level sequencer
fails when instruction sheet includes only identical operations?

Only if it is the latter (we are testing too low level details of
how sequencer code works, tying too tightly test with implementation)
the test should be removed.  I see that earlier test check that
sequencer handles correctly invalid instructions in todo.

> Therefore let's just get rid of the test that wants to verify that this
> limitation is still in place, in preparation for the upcoming work to
> teach the sequencer to do rebase -i's work.

Is it "upcoming work" as in one of the patches in this series?
If so, which patch?

> 
> Signed-off-by: Johannes Schindelin 
> ---
>  t/t3510-cherry-pick-sequence.sh | 11 ---
>  1 file changed, 11 deletions(-)
> 
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 7b7a89d..6465edf 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -459,17 +459,6 @@ test_expect_success 'malformed instruction sheet 1' '
>   test_expect_code 128 git cherry-pick --continue
>  '
>  
> -test_expect_success 'malformed instruction sheet 2' '

Hmmm... the description is somewhat lacking (especially compared to
the rest of test), anyway.

BTW. we should probably rename 'malformed instruction sheet 2'
to 'malformed instruction sheet' if there are no further such
tests after this removal, isn't it?

> - pristine_detach initial &&
> - test_expect_code 1 git cherry-pick base..anotherpick &&
> - echo "resolved" >foo &&
> - git add foo &&
> - git commit &&
> - sed "s/pick/revert/" .git/sequencer/todo >new_sheet &&
> - cp new_sheet .git/sequencer/todo &&
> - test_expect_code 128 git cherry-pick --continue
> -'
> -
>  test_expect_success 'empty commit set' '
>   pristine_detach initial &&
>   test_expect_code 128 git cherry-pick base..base
> 

Re: [PATCH v6 12/13] convert: add filter..process option

[Jakub Narębski]

Hi,

W dniu 31.08.2016 o 06:57, Torsten Bögershausen pisze:
> On Tue, Aug 30, 2016 at 03:23:10PM -0700, Junio C Hamano wrote:
>> Lars Schneider  writes:
>>> On 30 Aug 2016, at 20:59, Junio C Hamano  wrote:

Torsten, could you please in the future remove irrelevant parts of the
cited email you are responding to?  Thanks in advance,

[...]
 I meant it as primarily an example people can learn from when they
 want to write their own.
>>>
>>> I think `t/t0021/rot13-filter.pl` (part of this patch) serves this purpose 
>>> already.
>>
>> I would expect them to peek at contrib/, but do you seriously expect
>> people to comb through t/ directory?
> 
> How about a filter written in C, and placed in ./t/helper/ ?
>
> At least I feel that a filter in C-language could be a starting point
> for others which prefer, depending on the task the filter is going to do,
> to use shell scripts, perl, python or any other high-level language.

People do not look into t/helper/ area, but they do into contrib/.
Git provides examples on how to use its features there (among other
things).  Examples for how use the fast-import feature / protocol
are here, in contrib/fast-import.

I think that C language would be not a good choice, as required
low level wrangling of the protocol would obscure the relevant parts,
... well perhaps except if pkt-line was reused.  High-level language
would be better.  The contrib/fast-import directory includes examples
in shell script, Perl and Python.  The same could be done here
(possibly with more languages).
 
> A test case, where data can not be filtered, would be a minimum.
> As Jakub pointed out, you can use iconv with good and bad data.

The iconv might be good example, but I think it is not a practical
one; are there development environments that do not handle UTF-8?

More practical would be to have for example LFS-ish solution of
storing files as-is in a specified directory.  ROT13 is also good,
if not a very practical example.  Or rezipping all ZIP-compression
based files (ODF, JAR/WAR, DOCX / OOXML, CBZ, EPUB, APK, etc.).

-- 
Jakub Narębski

[PATCH v2 11/11] i18n: difftool: mark warnings for translation

[Vasco Almeida]

Signed-off-by: Vasco Almeida 
---
 git-difftool.perl | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index ebd13ba..de8d783 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -22,6 +22,7 @@ use File::Path qw(mkpath rmtree);
 use File::Temp qw(tempdir);
 use Getopt::Long qw(:config pass_through);
 use Git;
+use Git::I18N;
 
 sub usage
 {
@@ -144,7 +145,7 @@ sub setup_dir_diff
my $i = 0;
while ($i < $#rawdiff) {
if ($rawdiff[$i] =~ /^::/) {
-   warn << 'EOF';
+   warn __ <<'EOF';
 Combined diff formats ('-c' and '--cc') are not supported in
 directory diff mode ('-d' and '--dir-diff').
 EOF
@@ -451,11 +452,11 @@ sub dir_diff
}
 
if (exists $wt_modified{$file} and exists $tmp_modified{$file}) 
{
-   my $errmsg = "warning: Both files modified: ";
-   $errmsg .= "'$workdir/$file' and '$b/$file'.\n";
-   $errmsg .= "warning: Working tree file has been 
left.\n";
-   $errmsg .= "warning:\n";
-   warn $errmsg;
+   warn sprintf(__(
+   "warning: Both files modified:\n" .
+   "'%s/%s' and '%s/%s'.\n" .
+   "warning: Working tree file has been left.\n" .
+   "warning:\n"), $workdir, $file, $b, $file);
$error = 1;
} elsif (exists $tmp_modified{$file}) {
my $mode = stat("$b/$file")->mode;
@@ -467,8 +468,9 @@ sub dir_diff
}
}
if ($error) {
-   warn "warning: Temporary files exist in '$tmpdir'.\n";
-   warn "warning: You may want to cleanup or recover these.\n";
+   warn sprintf(__(
+   "warning: Temporary files exist in '%s'.\n" .
+   "warning: You may want to cleanup or recover 
these.\n"), $tmpdir);
exit(1);
} else {
exit_cleanup($tmpdir, $rc);
-- 
2.7.4

[PATCH v2 10/11] i18n: send-email: mark string with interpolation for translation

[Vasco Almeida]

Mark warnings, errors and other messages that are interpolated for
translation.

We must call sprintf() before calling die() and in few other
circumstances in order to interpolation take place.

Signed-off-by: Vasco Almeida 
---
 git-send-email.perl | 71 -
 1 file changed, 38 insertions(+), 33 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index e7f712e..c29381b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -279,10 +279,13 @@ sub signal_handler {
# tmp files from --compose
if (defined $compose_filename) {
if (-e $compose_filename) {
-   print "'$compose_filename' contains an intermediate 
version of the email you were composing.\n";
+   printf __("'%s' contains an intermediate version ".
+ "of the email you were composing.\n"),
+ $compose_filename;
}
if (-e ($compose_filename . ".final")) {
-   print "'$compose_filename.final' contains the composed 
email.\n"
+   printf __("'%s.final' contains the composed email.\n"),
+ $compose_filename;
}
}
 
@@ -431,7 +434,7 @@ $smtp_encryption = '' unless (defined $smtp_encryption);
 my(%suppress_cc);
 if (@suppress_cc) {
foreach my $entry (@suppress_cc) {
-   die "Unknown --suppress-cc field: '$entry'\n"
+   die sprintf(__("Unknown --suppress-cc field: '%s'\n"), $entry)
unless $entry =~ 
/^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/;
$suppress_cc{$entry} = 1;
}
@@ -460,7 +463,7 @@ my $confirm_unconfigured = !defined $confirm;
 if ($confirm_unconfigured) {
$confirm = scalar %suppress_cc ? 'compose' : 'auto';
 };
-die "Unknown --confirm setting: '$confirm'\n"
+die sprintf(__("Unknown --confirm setting: '%s'\n"), $confirm)
unless $confirm =~ /^(?:auto|cc|compose|always|never)/;
 
 # Debugging, print out the suppressions.
@@ -492,16 +495,16 @@ my %aliases;
 sub parse_sendmail_alias {
local $_ = shift;
if (/"/) {
-   print STDERR "warning: sendmail alias with quotes is not 
supported: $_\n";
+   printf STDERR __("warning: sendmail alias with quotes is not 
supported: %s\n"), $_;
} elsif (/:include:/) {
-   print STDERR "warning: `:include:` not supported: $_\n";
+   printf STDERR __("warning: `:include:` not supported: %s\n"), 
$_;
} elsif (/[\/|]/) {
-   print STDERR "warning: `/file` or `|pipe` redirection not 
supported: $_\n";
+   printf STDERR __("warning: `/file` or `|pipe` redirection not 
supported: %s\n"), $_;
} elsif (/^(\S+?)\s*:\s*(.+)$/) {
my ($alias, $addr) = ($1, $2);
$aliases{$alias} = [ split_addrs($addr) ];
} else {
-   print STDERR "warning: sendmail line is not recognized: $_\n";
+   printf STDERR __("warning: sendmail line is not recognized: 
%s\n"), $_;
}
 }
 
@@ -582,13 +585,12 @@ sub is_format_patch_arg {
if (defined($format_patch)) {
return $format_patch;
}
-   die(<

[PATCH v2 06/11] i18n: add--interactive: i18n of help_patch_cmd

[Vasco Almeida]

Mark help message of help_patch_cmd for translation.  The message must
be unfolded to be free of variables so we can have high quality
translations.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 65 +++
 1 file changed, 54 insertions(+), 11 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 5b89b97..acbfa4e 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1179,15 +1179,58 @@ sub edit_hunk_loop {
 }
 
 sub help_patch_cmd {
-   my $verb = lc $patch_mode_flavour{VERB};
-   my $target = $patch_mode_flavour{TARGET};
-   print colored $help_color, <

[PATCH v2 05/11] i18n: add--interactive: mark message for translation

[Vasco Almeida]

Mark message assembled in place for translation, unfolding each use case
for each entry in the %patch_modes hash table.

Previously, this script relied on whether $patch_mode was set to run the
command patch_update_cmd() or show status and loop the main loop. Now,
it uses $cmd to indicate we must run patch_update_cmd() and $patch_mode
is used to tell which flavor of the %patch_modes are we on.  This is
introduced in order to be able to mark and unfold the message prompt
knowing in which context we are.

The tracking of context was done previously by point %patch_mode_flavour
hash table to the correct entry of %patch_modes, focusing only on value
of %patch_modes. Now, we are also interested in the key ('staged',
'stash', 'checkout_head', ...).

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 91 ++-
 1 file changed, 83 insertions(+), 8 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 08badfa..5b89b97 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -91,6 +91,7 @@ sub colored {
 }
 
 # command line options
+my $cmd;
 my $patch_mode;
 my $patch_mode_revision;
 
@@ -171,7 +172,8 @@ my %patch_modes = (
},
 );
 
-my %patch_mode_flavour = %{$patch_modes{stage}};
+$patch_mode = 'stage';
+my %patch_mode_flavour = %{$patch_modes{$patch_mode}};
 
 sub run_cmd_pipe {
if ($^O eq 'MSWin32') {
@@ -1372,12 +1374,84 @@ sub patch_update_file {
for (@{$hunk[$ix]{DISPLAY}}) {
print;
}
-   print colored $prompt_color, $patch_mode_flavour{VERB},
- ($hunk[$ix]{TYPE} eq 'mode' ? ' mode change' :
-  $hunk[$ix]{TYPE} eq 'deletion' ? ' deletion' :
-  ' this hunk'),
- $patch_mode_flavour{TARGET},
- " [y,n,q,a,d,/$other,?]? ";
+   if ($patch_mode eq 'stage') {
+   if ($hunk[$ix]{TYPE} eq 'mode') {
+ print colored $prompt_color,
+   sprintf(__("Stage mode change [y,n,q,a,d,/%s,?]? 
"), $other);
+   } elsif ($hunk[$ix]{TYPE} eq 'deletion') {
+ print colored $prompt_color,
+   sprintf(__("Stage deletion [y,n,q,a,d,/%s,?]? "), 
$other);
+   } else {
+ print colored $prompt_color,
+   sprintf(__("Stage this hunk [y,n,q,a,d,/%s,?]? "), 
$other);
+   }
+   } elsif ($patch_mode eq 'stash') {
+   if ($hunk[$ix]{TYPE} eq 'mode') {
+ print colored $prompt_color,
+   sprintf(__("Stash mode change [y,n,q,a,d,/%s,?]? 
"), $other);
+   } elsif ($hunk[$ix]{TYPE} eq 'deletion') {
+ print colored $prompt_color,
+   sprintf(__("Stash deletion [y,n,q,a,d,/%s,?]? "), 
$other);
+   } else {
+ print colored $prompt_color,
+   sprintf(__("Stash this hunk [y,n,q,a,d,/%s,?]? "), 
$other);
+   }
+   } elsif ($patch_mode eq 'reset_head') {
+   if ($hunk[$ix]{TYPE} eq 'mode') {
+ print colored $prompt_color,
+   sprintf(__("Unstage mode change [y,n,q,a,d,/%s,?]? 
"), $other);
+   } elsif ($hunk[$ix]{TYPE} eq 'deletion') {
+ print colored $prompt_color,
+   sprintf(__("Unstage deletion [y,n,q,a,d,/%s,?]? "), 
$other);
+   } else {
+ print colored $prompt_color,
+   sprintf(__("Unstage this hunk [y,n,q,a,d,/%s,?]? 
"), $other);
+   }
+   } elsif ($patch_mode eq 'reset_nothead') {
+   if ($hunk[$ix]{TYPE} eq 'mode') {
+ print colored $prompt_color,
+   sprintf(__("Apply mode change to index 
[y,n,q,a,d,/%s,?]? "), $other);
+   } elsif ($hunk[$ix]{TYPE} eq 'deletion') {
+ print colored $prompt_color,
+   sprintf(__("Apply deletion to index 
[y,n,q,a,d,/%s,?]? "), $other);
+   } else {
+ print colored $prompt_color,
+   sprintf(__("Apply this hunk to index 
[y,n,q,a,d,/%s,?]? "), $other);
+   }
+   } elsif ($patch_mode eq 'checkout_index') {
+   if ($hunk[$ix]{TYPE} eq 'mode') {
+ print colored $prompt_color,
+   sprintf(__("Discard mode change from worktree 
[y,n,q,a,d,/%s,?]? "), $other);
+   } elsif ($hunk[$ix]{TYPE} eq 'deletion') {
+

[PATCH v2 01/11] i18n: add--interactive: mark strings for translation

[Vasco Almeida]

Mark simple strings (without interpolation) for translation.

Brackets around first parameter of ternary operator is necessary because
otherwise xgettext fails to extract strings marked for translation from
the rest of the file.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 68 +--
 1 file changed, 36 insertions(+), 32 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 822f857..fb8e5de 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -4,6 +4,7 @@ use 5.008;
 use strict;
 use warnings;
 use Git;
+use Git::I18N;
 
 binmode(STDOUT, ":raw");
 
@@ -252,7 +253,7 @@ sub list_untracked {
 }
 
 my $status_fmt = '%12s %12s %s';
-my $status_head = sprintf($status_fmt, 'staged', 'unstaged', 'path');
+my $status_head = sprintf($status_fmt, __('staged'), __('unstaged'), 
__('path'));
 
 {
my $initial;
@@ -678,7 +679,7 @@ sub update_cmd {
my @mods = list_modified('file-only');
return if (!@mods);
 
-   my @update = list_and_choose({ PROMPT => 'Update',
+   my @update = list_and_choose({ PROMPT => __('Update'),
   HEADER => $status_head, },
 @mods);
if (@update) {
@@ -690,7 +691,7 @@ sub update_cmd {
 }
 
 sub revert_cmd {
-   my @update = list_and_choose({ PROMPT => 'Revert',
+   my @update = list_and_choose({ PROMPT => __('Revert'),
   HEADER => $status_head, },
 list_modified());
if (@update) {
@@ -724,13 +725,13 @@ sub revert_cmd {
 }
 
 sub add_untracked_cmd {
-   my @add = list_and_choose({ PROMPT => 'Add untracked' },
+   my @add = list_and_choose({ PROMPT => __('Add untracked') },
  list_untracked());
if (@add) {
system(qw(git update-index --add --), @add);
say_n_paths('added', @add);
} else {
-   print "No untracked files.\n";
+   print __("No untracked files.\n");
}
print "\n";
 }
@@ -1159,8 +1160,11 @@ sub edit_hunk_loop {
}
else {
prompt_yesno(
-   'Your edited hunk does not apply. Edit again '
-   . '(saying "no" discards!) [y/n]? '
+   # TRANSLATORS: do not translate [y/n]
+   # The program will only accept that input
+   # at this point.
+   __('Your edited hunk does not apply. Edit again 
'
+  . '(saying "no" discards!) [y/n]? ')
) or return undef;
}
}
@@ -1206,11 +1210,11 @@ sub apply_patch_for_checkout_commit {
run_git_apply 'apply '.$reverse, @_;
return 1;
} elsif (!$applies_index) {
-   print colored $error_color, "The selected hunks do not apply to 
the index!\n";
-   if (prompt_yesno "Apply them to the worktree anyway? ") {
+   print colored $error_color, __("The selected hunks do not apply 
to the index!\n");
+   if (prompt_yesno __("Apply them to the worktree anyway? ")) {
return run_git_apply 'apply '.$reverse, @_;
} else {
-   print colored $error_color, "Nothing was applied.\n";
+   print colored $error_color, __("Nothing was 
applied.\n");
return 0;
}
} else {
@@ -1230,9 +1234,9 @@ sub patch_update_cmd {
 
if (!@mods) {
if (@all_mods) {
-   print STDERR "Only binary files changed.\n";
+   print STDERR __("Only binary files changed.\n");
} else {
-   print STDERR "No changes.\n";
+   print STDERR __("No changes.\n");
}
return 0;
}
@@ -1390,12 +1394,12 @@ sub patch_update_file {
my $response = $1;
my $no = $ix > 10 ? $ix - 10 : 0;
while ($response eq '') {
-   my $extra = "";
$no = display_hunks(\@hunk, $no);
if ($no < $num) {
-   $extra = " ( to see more)";
+   print __("go to which hunk 
( to see more)? ");
+   } else {
+   print __("go to which hunk? ");
}
-   print "go to which hunk$extra? ";
  

[PATCH v2 09/11] i18n: send-email: mark warnings and errors for translation

[Vasco Almeida]

Mark warnings, errors and other messages for translation.

Signed-off-by: Vasco Almeida 
---
 git-send-email.perl | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2521832..e7f712e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -118,20 +118,20 @@ sub format_2822_time {
my $localmin = $localtm[1] + $localtm[2] * 60;
my $gmtmin = $gmttm[1] + $gmttm[2] * 60;
if ($localtm[0] != $gmttm[0]) {
-   die "local zone differs from GMT by a non-minute interval\n";
+   die __("local zone differs from GMT by a non-minute 
interval\n");
}
if ((($gmttm[6] + 1) % 7) == $localtm[6]) {
$localmin += 1440;
} elsif ((($gmttm[6] - 1) % 7) == $localtm[6]) {
$localmin -= 1440;
} elsif ($gmttm[6] != $localtm[6]) {
-   die "local time offset greater than or equal to 24 hours\n";
+   die __("local time offset greater than or equal to 24 hours\n");
}
my $offset = $localmin - $gmtmin;
my $offhour = $offset / 60;
my $offmin = abs($offset % 60);
if (abs($offhour) >= 24) {
-   die ("local time offset greater than or equal to 24 hours\n");
+   die __("local time offset greater than or equal to 24 hours\n");
}
 
return sprintf("%s, %2d %s %d %02d:%02d:%02d %s%02d%02d",
@@ -199,13 +199,13 @@ sub do_edit {
map {
system('sh', '-c', $editor.' "$@"', $editor, $_);
if (($? & 127) || ($? >> 8)) {
-   die("the editor exited uncleanly, aborting 
everything");
+   die(__("the editor exited uncleanly, aborting 
everything"));
}
} @_;
} else {
system('sh', '-c', $editor.' "$@"', $editor, @_);
if (($? & 127) || ($? >> 8)) {
-   die("the editor exited uncleanly, aborting everything");
+   die(__("the editor exited uncleanly, aborting 
everything"));
}
}
 }
@@ -299,7 +299,7 @@ my $help;
 my $rc = GetOptions("h" => \$help,
 "dump-aliases" => \$dump_aliases);
 usage() unless $rc;
-die "--dump-aliases incompatible with other options\n"
+die __("--dump-aliases incompatible with other options\n")
 if !$help and $dump_aliases and @ARGV;
 $rc = GetOptions(
"sender|from=s" => \$sender,
@@ -362,7 +362,7 @@ unless ($rc) {
 usage();
 }
 
-die "Cannot run git format-patch from outside a repository\n"
+die __("Cannot run git format-patch from outside a repository\n")
if $format_patch and not $repo;
 
 # Now, let's fill any that aren't set in with defaults:
@@ -617,7 +617,7 @@ while (defined(my $f = shift @ARGV)) {
 }
 
 if (@rev_list_opts) {
-   die "Cannot run git format-patch from outside a repository\n"
+   die __("Cannot run git format-patch from outside a repository\n")
unless $repo;
push @files, $repo->command('format-patch', '-o', tempdir(CLEANUP => 
1), @rev_list_opts);
 }
@@ -636,7 +636,7 @@ if (@files) {
print $_,"\n" for (@files);
}
 } else {
-   print STDERR "\nNo patch files specified!\n\n";
+   print STDERR __("\nNo patch files specified!\n\n");
usage();
 }
 
@@ -728,7 +728,7 @@ EOT
$sender = $1;
next;
} elsif (/^(?:To|Cc|Bcc):/i) {
-   print "To/Cc/Bcc fields are not interpreted yet, they 
have been ignored\n";
+   print __("To/Cc/Bcc fields are not interpreted yet, 
they have been ignored\n");
next;
}
print $c2 $_;
@@ -737,7 +737,7 @@ EOT
close $c2;
 
if ($summary_empty) {
-   print "Summary email is empty, skipping it\n";
+   print __("Summary email is empty, skipping it\n");
$compose = -1;
}
 } elsif ($annotate) {
@@ -1313,7 +1313,7 @@ Message-Id: $message_id
$_ = ask(__("Send this email? ([y]es|[n]o|[q]uit|[a]ll): "),
 valid_re => qr/^(?:yes|y|no|n|quit|q|all|a)/i,
 default => $ask_default);
-   die "Send this email reply required" unless defined $_;
+   die __("Send this email reply required") unless defined $_;
if (/^n/i) {
return 0;
} elsif (/^q/i) {
@@ -1339,7 +1339,7 @@ Message-Id: $message_id
} else {
 
if (!defined $smtp_server) {
-   die "The required SMTP server is not properly defined."
+   die __("The required SMTP server is not properly 
defined.")
}
 
  

[PATCH v2 03/11] i18n: add--interactive: mark strings with interpolation for translation

[Vasco Almeida]

Use of sprintf following die or error_msg is necessary for placeholder
substitution take place.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index e11a33d..4e1e857 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -612,12 +612,12 @@ sub list_and_choose {
else {
$bottom = $top = find_unique($choice, @stuff);
if (!defined $bottom) {
-   error_msg "Huh ($choice)?\n";
+   error_msg sprintf(__("Huh (%s)?\n"), 
$choice);
next TOPLOOP;
}
}
if ($opts->{SINGLETON} && $bottom != $top) {
-   error_msg "Huh ($choice)?\n";
+   error_msg sprintf(__("Huh (%s)?\n"), $choice);
next TOPLOOP;
}
for ($i = $bottom-1; $i <= $top-1; $i++) {
@@ -1048,7 +1048,7 @@ sub edit_hunk_manually {
my $hunkfile = $repo->repo_path . "/addp-hunk-edit.diff";
my $fh;
open $fh, '>', $hunkfile
-   or die "failed to open hunk edit file for writing: " . $!;
+   or die sprintf(__("failed to open hunk edit file for writing: 
%s"), $!);
print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
print $fh @$oldtext;
my $participle = $patch_mode_flavour{PARTICIPLE};
@@ -1075,7 +1075,7 @@ EOF
}
 
open $fh, '<', $hunkfile
-   or die "failed to open hunk edit file for reading: " . $!;
+   or die sprintf(__("failed to open hunk edit file for reading: 
%s"), $!);
my @newtext = grep { !/^#/ } <$fh>;
close $fh;
unlink $hunkfile;
@@ -1225,7 +1225,7 @@ sub apply_patch_for_checkout_commit {
 
 sub patch_update_cmd {
my @all_mods = list_modified($patch_mode_flavour{FILTER});
-   error_msg "ignoring unmerged: $_->{VALUE}\n"
+   error_msg sprintf(__("ignoring unmerged: %s\n"), $_->{VALUE})
for grep { $_->{UNMERGED} } @all_mods;
@all_mods = grep { !$_->{UNMERGED} } @all_mods;
 
@@ -1407,11 +1407,13 @@ sub patch_update_file {
chomp $response;
}
if ($response !~ /^\s*\d+\s*$/) {
-   error_msg "Invalid number: 
'$response'\n";
+   error_msg sprintf(__("Invalid number: 
'%s'\n"),
+$response);
} elsif (0 < $response && $response <= $num) {
$ix = $response - 1;
} else {
-   error_msg "Sorry, only $num hunks 
available.\n";
+   error_msg sprintf(__("Sorry, only %s 
hunks available.\n"),
+$num);
}
next;
}
@@ -1449,7 +1451,7 @@ sub patch_update_file {
if ($@) {
my ($err,$exp) = ($@, $1);
$err =~ s/ at .*git-add--interactive 
line \d+,  line \d+.*$//;
-   error_msg "Malformed search regexp 
$exp: $err\n";
+   error_msg sprintf(__("Malformed search 
regexp %s: %s\n"), $exp, $err);
next;
}
my $iy = $ix;
@@ -1612,18 +1614,18 @@ sub process_args {
$patch_mode = $1;
$arg = shift @ARGV or die __("missing --");
} else {
-   die "unknown --patch mode: $1";
+   die sprintf(__("unknown --patch mode: %s"), $1);
}
} else {
$patch_mode = 'stage';
$arg = shift @ARGV or die __("missing --");
}
-   die "invalid argument $arg, expecting --"
-   unless $arg eq "--";
+   die sprintf(__("invalid argument %s, expecting --"),
+  $arg) unless $arg eq "--";
%patch_mode_flavour = %{$patch_modes{$patch_mode}};
}
elsif ($arg ne "--") {
-   die "invalid argument $arg, expecting --";
+   die 

[PATCH v2 04/11] i18n: add--interactive: mark plural strings

[Vasco Almeida]

Mark plural strings for translation.  Unfold each action case in one
entire sentence.

Pass new keyword for xgettext to extract.

Update test to include new subrotine Q__() for plural strings handling.

Signed-off-by: Vasco Almeida 
---
 Makefile  |  3 ++-
 git-add--interactive.perl | 24 
 perl/Git/I18N.pm  |  4 +++-
 t/t0202/test.pl   | 11 ++-
 4 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index de5a030..eedf1fa 100644
--- a/Makefile
+++ b/Makefile
@@ -2061,7 +2061,8 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
--keyword=_ --keyword=N_ --keyword="Q_:1,2"
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
--keyword=gettextln --keyword=eval_gettextln
-XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
+XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
+   --keyword=__ --keyword="Q__:1,2"
 LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
 LOCALIZED_SH = $(SCRIPT_SH) git-parse-remote.sh
 LOCALIZED_PERL = $(SCRIPT_PERL)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 4e1e857..08badfa 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -666,12 +666,18 @@ sub status_cmd {
 sub say_n_paths {
my $did = shift @_;
my $cnt = scalar @_;
-   print "$did ";
-   if (1 < $cnt) {
-   print "$cnt paths\n";
-   }
-   else {
-   print "one path\n";
+   if ($did eq 'added') {
+   printf(Q__("added one path\n", "added %d paths\n",
+  $cnt), $cnt);
+   } elsif ($did eq 'updated') {
+   printf(Q__("updated one path\n", "updated %d paths\n",
+  $cnt), $cnt);
+   } elsif ($did eq 'reversed') {
+   printf(Q__("reversed one path\n", "reversed %d paths\n",
+  $cnt), $cnt);
+   } else {
+   printf(Q__("touched one path\n", "touched %d paths\n",
+  $cnt), $cnt);
}
 }
 
@@ -1508,8 +1514,10 @@ sub patch_update_file {
elsif ($other =~ /s/ && $line =~ /^s/) {
my @split = split_hunk($hunk[$ix]{TEXT}, 
$hunk[$ix]{DISPLAY});
if (1 < @split) {
-   print colored $header_color, "Split 
into ",
-   scalar(@split), " hunks.\n";
+   print colored $header_color, sprintf(
+   Q__("Split into %d hunk.\n",
+   "Split into %d hunks.\n",
+   scalar(@split)), 
scalar(@split));
}
splice (@hunk, $ix, 1, @split);
$num = scalar @hunk;
diff --git a/perl/Git/I18N.pm b/perl/Git/I18N.pm
index f889fd6..5a75dd5 100644
--- a/perl/Git/I18N.pm
+++ b/perl/Git/I18N.pm
@@ -13,7 +13,7 @@ BEGIN {
}
 }
 
-our @EXPORT = qw(__);
+our @EXPORT = qw(__ Q__);
 our @EXPORT_OK = @EXPORT;
 
 sub __bootstrap_locale_messages {
@@ -44,6 +44,7 @@ BEGIN
eval {
__bootstrap_locale_messages();
*__ = \::Messages::gettext;
+   *Q__ = \::Messages::ngettext;
1;
} or do {
# Tell test.pl that we couldn't load the gettext library.
@@ -51,6 +52,7 @@ BEGIN
 
# Just a fall-through no-op
*__ = sub ($) { $_[0] };
+   *Q__ = sub ($$$) { $_[2] == 1 ? $_[0] : $_[1] };
};
 }
 
diff --git a/t/t0202/test.pl b/t/t0202/test.pl
index 2c10cb4..98aa9a3 100755
--- a/t/t0202/test.pl
+++ b/t/t0202/test.pl
@@ -4,7 +4,7 @@ use lib (split(/:/, $ENV{GITPERLLIB}));
 use strict;
 use warnings;
 use POSIX qw(:locale_h);
-use Test::More tests => 8;
+use Test::More tests => 11;
 use Git::I18N;
 
 my $has_gettext_library = $Git::I18N::__HAS_LIBRARY;
@@ -31,6 +31,7 @@ is_deeply(\@Git::I18N::EXPORT, \@Git::I18N::EXPORT_OK, 
"sanity: Git::I18N export
# more gettext wrapper functions.
my %prototypes = (qw(
__  $
+   Q__ $$$
));
while (my ($sub, $proto) = each %prototypes) {
is(prototype(\&{"Git::I18N::$sub"}), $proto, "sanity: $sub has 
a $proto prototype");
@@ -46,6 +47,14 @@ is_deeply(\@Git::I18N::EXPORT, \@Git::I18N::EXPORT_OK, 
"sanity: Git::I18N export
my ($got, $expect) = (('TEST: A Perl test string') x 2);
 
is(__($got), $expect, "Passing a string through __() in the C locale 
works");
+
+   my ($got_singular, $got_plural, $expect_singular, $expect_plural) =
+   (('TEST: 1 file', 'TEST: n files') x 2);
+
+   is(Q__($got_singular, $got_plural, 1), $expect_singular,
+   

[PATCH v2 07/11] i18n: add--interactive: mark edit_hunk_manually message for translation

[Vasco Almeida]

Mark message of edit_hunk_manually displayed in the editing file when
user chooses 'e' option.  The message had to be unfolded to allow
translation of the $participle verb.

Some messages end up being exactly the same for some uses cases, but
left it for easier change in the future, e.g., wanting to change wording
of one particular use case.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 60 ++-
 1 file changed, 49 insertions(+), 11 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index acbfa4e..235142c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1057,22 +1057,60 @@ sub edit_hunk_manually {
my $fh;
open $fh, '>', $hunkfile
or die sprintf(__("failed to open hunk edit file for writing: 
%s"), $!);
-   print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
+   print $fh __("# Manual hunk edit mode -- see bottom for a quick 
guide\n");
print $fh @$oldtext;
-   my $participle = $patch_mode_flavour{PARTICIPLE};
my $is_reverse = $patch_mode_flavour{IS_REVERSE};
my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') : ('+', 
'-');
-   print $fh <

[PATCH v2 08/11] i18n: send-email: mark strings for translation

[Vasco Almeida]

Mark strings often displayed to user for translation.

Signed-off-by: Vasco Almeida 
---
 git-send-email.perl | 53 +++--
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 6958785..2521832 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -28,6 +28,7 @@ use File::Temp qw/ tempdir tempfile /;
 use File::Spec::Functions qw(catfile);
 use Error qw(:try);
 use Git;
+use Git::I18N;
 
 Getopt::Long::Configure qw/ pass_through /;
 
@@ -795,12 +796,12 @@ foreach my $f (@files) {
 }
 
 if (!defined $auto_8bit_encoding && scalar %broken_encoding) {
-   print "The following files are 8bit, but do not declare " .
-   "a Content-Transfer-Encoding.\n";
+   print __("The following files are 8bit, but do not declare " .
+"a Content-Transfer-Encoding.\n");
foreach my $f (sort keys %broken_encoding) {
print "$f\n";
}
-   $auto_8bit_encoding = ask("Which 8bit encoding should I declare 
[UTF-8]? ",
+   $auto_8bit_encoding = ask(__("Which 8bit encoding should I declare 
[UTF-8]? "),
  valid_re => qr/.{4}/, confirm_only => 1,
  default => "UTF-8");
 }
@@ -827,7 +828,7 @@ if (defined $sender) {
 # But it's a no-op to run sanitize_address on an already sanitized address.
 $sender = sanitize_address($sender);
 
-my $to_whom = "To whom should the emails be sent (if anyone)?";
+my $to_whom = __("To whom should the emails be sent (if anyone)?");
 my $prompting = 0;
 if (!@initial_to && !defined $to_cmd) {
my $to = ask("$to_whom ",
@@ -857,7 +858,7 @@ sub expand_one_alias {
 
 if ($thread && !defined $initial_reply_to && $prompting) {
$initial_reply_to = ask(
-   "Message-ID to be used as In-Reply-To for the first email (if 
any)? ",
+   __("Message-ID to be used as In-Reply-To for the first email 
(if any)? "),
default => "",
valid_re => qr/\@.*\./, confirm_only => 1);
 }
@@ -916,7 +917,10 @@ sub validate_address {
my $address = shift;
while (!extract_valid_address($address)) {
print STDERR "error: unable to extract a valid address from: 
$address\n";
-   $_ = ask("What to do with this address? ([q]uit|[d]rop|[e]dit): 
",
+   # TRANSLATORS: Make sure to include [q] [d] [e] in your
+   # translation. The program will only accept English input
+   # at this point.
+   $_ = ask(__("What to do with this address? 
([q]uit|[d]rop|[e]dit): "),
valid_re => qr/^(?:quit|q|drop|d|edit|e)/i,
default => 'q');
if (/^d/i) {
@@ -1291,17 +1295,22 @@ Message-Id: $message_id
if ($needs_confirm eq "inform") {
$confirm_unconfigured = 0; # squelch this message for 
the rest of this run
$ask_default = "y"; # assume yes on EOF since user 
hasn't explicitly asked for confirmation
-   print "The Cc list above has been expanded by 
additional\n";
-   print "addresses found in the patch commit message. 
By default\n";
-   print "send-email prompts before sending whenever 
this occurs.\n";
-   print "This behavior is controlled by the 
sendemail.confirm\n";
-   print "configuration setting.\n";
-   print "\n";
-   print "For additional information, run 'git 
send-email --help'.\n";
-   print "To retain the current behavior, but squelch 
this message,\n";
-   print "run 'git config --global sendemail.confirm 
auto'.\n\n";
+   print __(
+"The Cc list above has been expanded by additional
+addresses found in the patch commit message. By default
+send-email prompts before sending whenever this occurs.
+This behavior is controlled by the sendemail.confirm
+configuration setting.
+
+For additional information, run 'git send-email --help'.
+To retain the current behavior, but squelch this message,
+run 'git config --global sendemail.confirm auto'."),
+"\n\n";
}
-   $_ = ask("Send this email? ([y]es|[n]o|[q]uit|[a]ll): ",
+   # TRANSLATORS: Make sure to include [y] [n] [q] [a] in your
+   # translation. The program will only accept English input
+   # at this point.
+   $_ = ask(__("Send this email? ([y]es|[n]o|[q]uit|[a]ll): "),
 valid_re => qr/^(?:yes|y|no|n|quit|q|all|a)/i,
 default => $ask_default);
die "Send this email reply required" unless defined $_;
@@ -1403,7 +1412,7 @@ Message-Id: $message_id

[PATCH v2 02/11] i18n: add--interactive: mark simple here documents for translation

[Vasco Almeida]

Mark messages in here document without interpolation for translation.

Marking for translation by removing here documents this way, rather than
take advantage of "print __ << EOF" way, makes other instances of help
messages in clean.c match the first two in this file.  Otherwise,
reusing here document would add a trailer newline to the message, making
them not match 100%, hence creating two entries in pot template for
translation rather than a single entry.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index fb8e5de..e11a33d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -636,25 +636,25 @@ sub list_and_choose {
 }
 
 sub singleton_prompt_help_cmd {
-   print colored $help_color, <<\EOF ;
-Prompt help:
+   print colored $help_color, __(
+"Prompt help:
 1  - select a numbered item
 foo- select item based on unique prefix
-   - (empty) select nothing
-EOF
+   - (empty) select nothing"),
+"\n";
 }
 
 sub prompt_help_cmd {
-   print colored $help_color, <<\EOF ;
-Prompt help:
+   print colored $help_color, __(
+"Prompt help:
 1  - select a single item
 3-5- select a range of items
 2-3,6-9- select multiple ranges
 foo- select item based on unique prefix
 -...   - unselect specified items
 *  - choose all items
-   - (empty) finish selecting
-EOF
+   - (empty) finish selecting"),
+"\n";
 }
 
 sub status_cmd {
@@ -1573,14 +1573,14 @@ sub quit_cmd {
 }
 
 sub help_cmd {
-   print colored $help_color, <<\EOF ;
-status- show paths with changes
+   print colored $help_color, __(
+"status- show paths with changes
 update- add working tree state to the staged set of changes
 revert- revert staged set of changes back to the HEAD version
 patch - pick hunks and update selectively
 diff - view diff between HEAD and index
-add untracked - add contents of untracked files to the staged set of changes
-EOF
+add untracked - add contents of untracked files to the staged set of changes"),
+"\n";
 }
 
 sub process_args {
-- 
2.7.4

[PATCH v2 00/11] Mark strings in perl script for translation

[Vasco Almeida]

Mark messages in some perl scripts for translation.

Since v1, adds brackets so parameter grouping of sprintf parameters is easier
to see.

Interdiff included below.

Vasco Almeida (11):
  i18n: add--interactive: mark strings for translation
  i18n: add--interactive: mark simple here documents for translation
  i18n: add--interactive: mark strings with interpolation for
translation
  i18n: add--interactive: mark plural strings
  i18n: add--interactive: mark message for translation
  i18n: add--interactive: i18n of help_patch_cmd
  i18n: add--interactive: mark edit_hunk_manually message for
translation
  i18n: send-email: mark strings for translation
  i18n: send-email: mark warnings and errors for translation
  i18n: send-email: mark string with interpolation for translation
  i18n: difftool: mark warnings for translation

 Makefile  |   3 +-
 git-add--interactive.perl | 358 ++
 git-difftool.perl |  18 +--
 git-send-email.perl   | 160 +++--
 perl/Git/I18N.pm  |   4 +-
 t/t0202/test.pl   |  11 +-
 6 files changed, 376 insertions(+), 178 deletions(-)

 >8 
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 1652a57..235142c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -614,12 +614,12 @@ sub list_and_choose {
else {
$bottom = $top = find_unique($choice, @stuff);
if (!defined $bottom) {
-   error_msg sprintf __("Huh (%s)?\n"), 
$choice;
+   error_msg sprintf(__("Huh (%s)?\n"), 
$choice);
next TOPLOOP;
}
}
if ($opts->{SINGLETON} && $bottom != $top) {
-   error_msg sprintf __("Huh (%s)?\n"), $choice;
+   error_msg sprintf(__("Huh (%s)?\n"), $choice);
next TOPLOOP;
}
for ($i = $bottom-1; $i <= $top-1; $i++) {
@@ -669,17 +669,17 @@ sub say_n_paths {
my $did = shift @_;
my $cnt = scalar @_;
if ($did eq 'added') {
-   printf Q__("added one path\n", "added %d paths\n",
-  $cnt), $cnt;
+   printf(Q__("added one path\n", "added %d paths\n",
+  $cnt), $cnt);
} elsif ($did eq 'updated') {
-   printf Q__("updated one path\n", "updated %d paths\n",
-  $cnt), $cnt;
+   printf(Q__("updated one path\n", "updated %d paths\n",
+  $cnt), $cnt);
} elsif ($did eq 'reversed') {
-   printf Q__("reversed one path\n", "reversed %d paths\n",
-  $cnt), $cnt;
+   printf(Q__("reversed one path\n", "reversed %d paths\n",
+  $cnt), $cnt);
} else {
-   printf Q__("touched one path\n", "touched %d paths\n",
-  $cnt), $cnt;
+   printf(Q__("touched one path\n", "touched %d paths\n",
+  $cnt), $cnt);
}
 }
 
@@ -1056,12 +1056,12 @@ sub edit_hunk_manually {
my $hunkfile = $repo->repo_path . "/addp-hunk-edit.diff";
my $fh;
open $fh, '>', $hunkfile
-   or die sprintf __("failed to open hunk edit file for writing: 
%s"), $!;
+   or die sprintf(__("failed to open hunk edit file for writing: 
%s"), $!);
print $fh __("# Manual hunk edit mode -- see bottom for a quick 
guide\n");
print $fh @$oldtext;
my $is_reverse = $patch_mode_flavour{IS_REVERSE};
my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') : ('+', 
'-');
-   print $fh (sprintf __(
+   print $fh sprintf(__(
 "# ---
 # To remove '%s' lines, make them ' ' lines (context).
 # To remove '%s' lines, delete them.
@@ -1121,7 +1121,7 @@ sub edit_hunk_manually {
}
 
open $fh, '<', $hunkfile
-   or die sprintf __("failed to open hunk edit file for reading: 
%s"), $!;
+   or die sprintf(__("failed to open hunk edit file for reading: 
%s"), $!);
my @newtext = grep { !/^#/ } <$fh>;
close $fh;
unlink $hunkfile;
@@ -1314,7 +1314,7 @@ sub apply_patch_for_checkout_commit {
 
 sub patch_update_cmd {
my @all_mods = list_modified($patch_mode_flavour{FILTER});
-   error_msg sprintf __("ignoring unmerged: %s\n"), $_->{VALUE}
+   error_msg sprintf(__("ignoring unmerged: %s\n"), $_->{VALUE})
for grep { $_->{UNMERGED} } @all_mods;
@all_mods = grep { !$_->{UNMERGED} } @all_mods;
 
@@ -1458,100 +1458,79 @@ sub patch_update_file {
if ($patch_mode eq 'stage') {
if 

Re: [PATCH 20/20] builtin/reset: convert to use struct object_id

[Johannes Schindelin]

Hi Brian,

On Sun, 28 Aug 2016, brian m. carlson wrote:

> Signed-off-by: brian m. carlson 

I now looked through the rest of this patch series, and they all look good
(incidentally, I spotted the same update_ref() thing before reading Paul's
reply, so I guess my review was more thorough than usual, this time).

The conversion to OIDs seems to be very important to me, so I am very
happy that you keep the ball rolling!

Ciao,
Dscho

Re: [PATCH 10/20] notes-merge: convert struct notes_merge_pair to struct object_id

[Johannes Schindelin]

Hi Brian,

On Sun, 28 Aug 2016, brian m. carlson wrote:

>   assert(!"Invalid existing change recorded");
>   } else {
> - hashcpy(mp->obj, obj);
> - hashcpy(mp->base, p->one->oid.hash);
> - hashcpy(mp->local, uninitialized);
> - hashcpy(mp->remote, p->two->oid.hash);
> + hashcpy(mp->obj.hash, obj);
> + oidcpy(>base, >one->oid);
> + hashcpy(mp->local.hash, uninitialized);
> + oidcpy(>remote, >two->oid);
>   len++;

This puzzled me at first, and a little bit of digging revealed that obj
is a local variable and uninitialized is a file-local variable. I would
have thought that these would be converted to object_id's, too...

I guess that's left for later?

Ciao,
Dscho

Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository

[Jeff King]

On Wed, Aug 31, 2016 at 11:12:26AM +0200, Dennis Kaarsemaker wrote:

> That is indeed a bug. git reads the config of t1 and then thinks a
> template config has set that value, so it won't override it.

This is a regression in v2.9.0 due to my ae5f677 (lazily load
core.sharedrepository, 2016-03-11).

> This is caused by git init reading the config via
> get_shared_repository. The comment above it indicates that this may not
> be needed, and indeed not doing it makes this bug go away.

Hrm. I'm not sure if that will work, though, because we may call
get_shared_repository() from other code-paths (e.g., anything that calls
adjust_shared_perm(), like safe_create_leading_directories).

We may need to do something like turn off the
need_shared_repository_from_config in init-db, since I think it would
not want to ever read from the default config sources in most of its
code-paths (OTOH, it should in theory respect core.sharedRepository in
~/.gitconfig, so maybe there is another more elegant way of handling
this).

I'm out of time for the day, so it will be a while before I can dig
further. Please feel free to figure it out while I am sleeping. :)

-Peff

Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Johannes Schindelin]

Hi,

[Sverre: we are considering to remove the - suffix in test-results/,
see more below.]

On Tue, 30 Aug 2016, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Hmm, interesting. Your approach seems reasonable, but I have to wonder
> > if writing the pid in the first place is sane.
> >
> > I started to write up my reasoning in this email, but realized it was
> > rapidly becoming the content of a commit message. So here is that
> > commit.
> 
> Sounds sensible; if this makes Dscho's "which ones failed in the
> previous run" simpler, that is even better ;-)

I did not have the time to dig further before now. There must have been a
good reason why we append the PID.

Sverre, you added that code in 2d84e9f (Modify test-lib.sh to output stats
to t/test-results/*, 2008-06-08): any idea why the - suffix was
needed?

Ciao,
Dscho

Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Johannes Schindelin]

Hi Ævar,

On Tue, 30 Aug 2016, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Aug 30, 2016 at 10:51 PM, Jeff King  wrote:
> > On Tue, Aug 30, 2016 at 10:48:19PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >
> >> >  -failed: $(patsubst trash,,$(patsubst directory.%,%.sh,$(wildcard 
> >> > trash\ directory.t[0-9]*)))
> >> >  +failed:
> >> >  +  @failed=$$(cd '$(TEST_RESULTS_DIRECTORY_SQ)' && \
> >> >  +  grep -l '^failed [1-9]' $$(ls -t *.counts | \
> >> >  +  sed 
> >> > 'G;h;/^\(t[^.]*\)-[0-9]*\..*\n\1-[0-9]*\./d;P;d') | \
> >> >  +  sed -n 's/-[0-9]*\.counts$$/.sh/p') && \
> >> >  +  test -z "$$failed" || $(MAKE) $$failed
> >> >
> >> >   prove: pre-clean $(TEST_LINT)
> >> > @echo "*** prove ***"; $(PROVE) --exec '$(SHELL_PATH_SQ)' 
> >> > $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
> >>
> >> I don't at all mind this solution to the problem, if it works for that's 
> >> cool.
> >>
> >> But FWIW something you may have missed is that you can just use
> >> prove(1) for this, which is why I initially patched git.git to support
> >> TAP, so I didn't have to implement stuff like this.
> >
> > Heh. I think each iteration of this patch will be destined to have
> > somebody[1] point Johannes at prove. ;)
> >
> > (But I really do recommend prove if you can use it).
> >
> > -Peff
> >
> > [1] 
> > http://public-inbox.org/git/20160630063725.gc15...@sigill.intra.peff.net/
> 
> Sorry about that, I see it's been mentioned already.

Yeah, it is true that prove(1) would be able to help. If it worked
reliably on Windows. (Probably Perl's fault, not prove's.)

> My only excuse is that I don't know how to operate my E-Mail client :)

But we use email to discuss all things Git because it makes everything so
easy and convenient... ;-)

Ciao,
Dscho

Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository

[Dennis Kaarsemaker]

That is indeed a bug. git reads the config of t1 and then thinks a
template config has set that value, so it won't override it.

This is caused by git init reading the config via
get_shared_repository. The comment above it indicates that this may not
be needed, and indeed not doing it makes this bug go away.

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 3a45f0b..b2883a6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -494,14 +494,6 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
retry:
if (chdir(argv[0]) < 0) {
if (!mkdir_tried) {
-   int saved;
-   /*
-* At this point we haven't read any 
configuration,
-* and we know shared_repository should always 
be 0;
-* but just in case we play safe.
-*/
-   saved = get_shared_repository();
-   set_shared_repository(0);
switch 
(safe_create_leading_directories_const(argv[0])) {
case SCLD_OK:
case SCLD_PERMS:
@@ -513,7 +505,6 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
die_errno(_("cannot mkdir %s"), 
argv[0]);
break;
}
-   set_shared_repository(saved);
if (mkdir(argv[0], 0777) < 0)
die_errno(_("cannot mkdir %s"), 
argv[0]);
                                mkdir_tried = 1; 

On wo, 2016-08-31 at 10:09 +0200, doak wrote:
> Hi there,
> 
> If 'git init /path/to/repo' is called inside another repository with
> 'core.logallrefupdates' set to true, it will be not set by default in
> the created repository.
> This seems to be a bug.
> I am using Git v2.9.3 (Arch).
> 
> Steps to reproduce:
> -
> git init t1
> cd t1
> # 'core.logallrefupdates' will not be set for 't2'.
> git init ../t2
> -
> 
> Stated from 'git-config(1)':
> -
> core.logAllRefUpdates
> [...]
> This value is true by default in a repository that has a
> working directory associated with it, and false by default in a bare
> repository.
> -
> 
> 
> With best regards,
> doak
> 
> 

[PATCH 14/34] sequencer (rebase -i): update refs after a successful rebase

[Johannes Schindelin]

An interactive rebase operates on a detached HEAD (to keep the reflog
of the original branch relatively clean), and updates the branch only
at the end.

Now that the sequencer learns to perform interactive rebases, it also
needs to learn the trick to update the branch before removing the
directory containing the state of the interactive rebase.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 32 +++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index d4437f5..766 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -100,6 +100,8 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, 
"rebase-merge/stopped-sha")
 static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
 static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
 static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
+static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
+static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
 
 /* We will introduce the 'interactive rebase' mode later */
 static inline int is_rebase_i(const struct replay_opts *opts)
@@ -1769,12 +1771,39 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
}
 
if (is_rebase_i(opts)) {
-   struct strbuf buf = STRBUF_INIT;
+   struct strbuf head_ref = STRBUF_INIT, buf = STRBUF_INIT;
 
/* Stopped in the middle, as planned? */
if (todo_list->current < todo_list->nr)
return 0;
 
+   if (read_oneliner(_ref, rebase_path_head_name(), 0) &&
+   starts_with(head_ref.buf, "refs/")) {
+   unsigned char head[20], orig[20];
+
+   if (get_sha1("HEAD", head))
+   return error("Cannot read HEAD");
+   if (!read_oneliner(, rebase_path_orig_head(), 0) ||
+   get_sha1_hex(buf.buf, orig))
+   return error("Could not read orig-head");
+   strbuf_addf(, "rebase -i (finish): %s onto ",
+   head_ref.buf);
+   if (!read_oneliner(, rebase_path_onto(), 0))
+   return error("Could not read 'onto'");
+   if (update_ref(buf.buf, head_ref.buf, head, orig,
+   REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+   return error("Could not update %s",
+   head_ref.buf);
+   strbuf_reset();
+   strbuf_addf(,
+   "rebase -i (finish): returning to %s",
+   head_ref.buf);
+   if (create_symref("HEAD", head_ref.buf, buf.buf))
+   return error("Could not update HEAD to %s",
+   head_ref.buf);
+   strbuf_reset();
+   }
+
if (opts->verbose) {
const char *argv[] = {
"diff-tree", "--stat", NULL, NULL
@@ -1789,6 +1818,7 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
strbuf_reset();
}
strbuf_release();
+   strbuf_release(_ref);
}
 
/*
-- 
2.10.0.rc2.102.g5c102ec