Re: What's cooking in git.git (Aug 2016, #07; Thu, 18)

[Junio C Hamano]

On Fri, Aug 19, 2016 at 4:46 PM, Stefan Beller  wrote:
>>
>> * sb/submodule-clone-rr (2016-08-17) 8 commits
>>
>>  I spotted a last-minute bug in v5, which is not a very good sign
>>  (it shows that nobody is reviewing).  Any more comments?
>
> nobody except you ;)

OK, it shows that nobody competent enough to find easy bugs is reviewing ;-)
IOW, I do not trust a review in this area done by myself alone.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

BUG: Use GNU Short Option Style "clumping" rules not obeyed for `git stash`

[Damian Nowak]

```
% git stash -ku
error: unknown option for 'stash save': -ku
(...)
% git stash -k -u
Saved working directory and index state WIP on master: (...)
(...)
```

In other parts of git, clumping (as defined by [GNU Short Option Style
guide](https://www.gnu.org/software/tar/manual/html_node/Short-Options.html#SEC37))
works, for example:


-v for verbose, -m for message:
```
% git commit -vm 'xd'
[master a560195] xd
 1 file changed, 91 insertions(+), 51 deletions(-)
 rewrite spec/actions/hosting/meta_machine/upgrade_plan_spec.rb (96%)
```
-q for quiet, -m for message
```
% git commit -qm 'xd'
(no output)
```

-- 
Kind regards
Damian Nowak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 0/8] submodule show inline diff

[Stefan Beller]

On Fri, Aug 19, 2016 at 4:34 PM, Jacob Keller  wrote:
> From: Jacob Keller 
>
> More suggestions from Junio and a few changes to support submodule name
> lookup. Hopefully we're getting close to the goal!
>
> interdiff between v8 and current:
> diff --git c/builtin/rev-list.c w/builtin/rev-list.c
> index 21cde8dd6b31..8479f6ed28aa 100644
> --- c/builtin/rev-list.c
> +++ w/builtin/rev-list.c
> @@ -129,29 +129,29 @@ static void show_commit(struct commit *commit, void 
> *data)
> 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.)
> -   */
> +* 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 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)
> diff --git c/cache.h w/cache.h
> index da9f0be67d7b..70428e92d7ed 100644
> --- c/cache.h
> +++ w/cache.h
> @@ -953,24 +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)
>
> -extern const struct object_id empty_tree_oid;
>
>  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 c/diff.h w/diff.h
> index 

Re: What's cooking in git.git (Aug 2016, #07; Thu, 18)

[Stefan Beller]

>
> * sb/submodule-clone-rr (2016-08-17) 8 commits
>  - clone: recursive and reference option triggers submodule alternates
>  - clone: implement optional references
>  - clone: clarify option_reference as required
>  - clone: factor out checking for an alternate path
>  - submodule--helper update-clone: allow multiple references
>  - submodule--helper module-clone: allow multiple references
>  - t7408: merge short tests, factor out testing method
>  - t7408: modernize style
>
>  I spotted a last-minute bug in v5, which is not a very good sign
>  (it shows that nobody is reviewing).  Any more comments?

nobody except you ;)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] format-patch: show 0/1 and 1/1 for singleton patch with cover letter

[Jacob Keller]

From: Jacob Keller 

Change the default behavior of git-format-patch to generate numbered
sequence of 0/1 and 1/1 when generating both a cover-letter and a single
patch. This standardizes the cover letter to have 0/N which helps
distinguish the cover letter from the patch itself. Since the behavior
is easily changed via configuration as well as the use of -n and -N this
should be acceptable default behavior.

Signed-off-by: Jacob Keller 
---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 92dc34dcb0cc..8e6100fb0c5b 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1676,7 +1676,7 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
/* nothing to do */
return 0;
total = nr;
-   if (!keep_subject && auto_number && total > 1)
+   if (!keep_subject && auto_number && (total > 1 || cover_letter))
numbered = 1;
if (numbered)
rev.total = total + start_number - 1;
-- 
2.10.0.rc0.259.g83512d9

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 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 
---
 submodule.c | 115 +++-
 1 file changed, 82 insertions(+), 33 deletions(-)

diff --git a/submodule.c b/submodule.c
index 422353ccf6cc..1fb753af3066 100644
--- a/submodule.c
+++ b/submodule.c
@@ -278,9 +278,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);
@@ -289,13 +289,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,
@@ -333,31 +326,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);
@@ -365,11 +350,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 = lookup_commit_reference(one->hash);
+   *right = 

[PATCH v9 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 
---
 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 16253b191f53..b38d95eb249c 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 (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF &&
+  

[PATCH v9 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 
---
 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 d6b321da3d1d..16253b191f53 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 e1a51b7506ff..422353ccf6cc 100644
--- a/submodule.c
+++ b/submodule.c
@@ -335,7 +335,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)
 {
@@ -345,14 +345,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))
@@ -365,16 +365,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.rc0.259.g83512d9

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 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 
---
 cache.h | 25 +
 sha1_file.c |  6 ++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index f30a4417efdf..70428e92d7ed 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 1e23fc186a02..21cf923bcf1f 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.rc0.259.g83512d9

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 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 
---
 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 v9 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 534c12e28ea8..50bef1f07658 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 7883729edf10..747a204d75a4 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.rc0.259.g83512d9

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 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 
---
 diff.c | 12 ++--
 diff.h |  7 ++-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/diff.c b/diff.c
index e57cf39ad109..d6b321da3d1d 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 1f57aad25c71..43b353aea091 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.rc0.259.g83512d9

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 5/8] submodule: allow add_submodule_odb to work even if path is not checked out

[Jacob Keller]

From: Jacob Keller 

Currently, do_submodule_path will first try to locate the git directory
using read_gitfile on /.git. If this fails, it goes
ahead and assumes the path is actually the git directory. This is good
as it allows submodules which aren't stored in the superproject's .git
directory to function correctly. However, in some cases the submodule is
no longer locally checked out, but still has object data stored in the
parent project's .git/modules/.

To make this work, add code to check if we found a valid git directory.
If we haven't, then try the standard location of module data instead.
This has the advantage of allowing callers of do_submodule_path
(add_submodule_odb) to correctly function even if the submodule isn't
currently checked out, but was previously initialized.

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 that even once we remove the submodule directory, it
works by checking in the .git directory.

Signed-off-by: Jacob Keller 
---
 path.c|  16 
 submodule.c   |   2 +-
 t/t4059-diff-submodule-not-initialized.sh | 127 ++
 3 files changed, 144 insertions(+), 1 deletion(-)
 create mode 100755 t/t4059-diff-submodule-not-initialized.sh

diff --git a/path.c b/path.c
index fe3c4d96c6d8..081a22c1163c 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)
 {
@@ -474,6 +475,7 @@ static void do_submodule_path(struct strbuf *buf, const 
char *path,
const char *git_dir;
struct strbuf git_submodule_common_dir = STRBUF_INIT;
struct strbuf git_submodule_dir = STRBUF_INIT;
+   const struct submodule *submodule_config;
 
strbuf_addstr(buf, path);
strbuf_complete(buf, '/');
@@ -484,6 +486,20 @@ 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)) {
+   strbuf_reset(buf);
+   /*
+* Lookup the submodule name from the config. If that fails
+* fall back to assuming the path is the name.
+*/
+   submodule_config = submodule_from_path(null_sha1, path);
+   if (submodule_config)
+   strbuf_git_path(buf, "%s/%s", "modules",
+   submodule_config->name);
+   else
+   strbuf_git_path(buf, "%s/%s", "modules", path);
+   }
+
strbuf_addch(buf, '/');
strbuf_addbuf(_submodule_dir, buf);
 
diff --git a/submodule.c b/submodule.c
index 1b5cdfb7e784..e1a51b7506ff 100644
--- a/submodule.c
+++ b/submodule.c
@@ -348,7 +348,7 @@ void show_submodule_summary(FILE *f, const char *path,
if (is_null_sha1(two))
message = "(submodule deleted)";
else if (add_submodule_odb(path))
-   message = "(not checked out)";
+   message = "(not initialized)";
else if (is_null_sha1(one))
message = "(new submodule)";
else if (!(left = lookup_commit_reference(one)) ||
diff --git a/t/t4059-diff-submodule-not-initialized.sh 
b/t/t4059-diff-submodule-not-initialized.sh
new file mode 100755
index ..c8775854d3c2
--- /dev/null
+++ b/t/t4059-diff-submodule-not-initialized.sh
@@ -0,0 +1,127 @@
+#!/bin/sh
+#
+# Copyright (c) 2016 Jacob Keller, based on t4041 by Jens Lehmann
+#
+
+test_description='Test for submodule diff on non-checked out submodule
+
+This test tries to verify that add_submodule_odb works when the submodule was
+initialized previously but the checkout has since been removed.
+'
+
+. ./test-lib.sh
+
+# Tested non-UTF-8 encoding
+test_encoding="ISO8859-1"
+
+# String "added" in German (translated with Google Translate), encoded in 
UTF-8,
+# used in sample commit log messages in add_file() function below.
+added=$(printf "hinzugef\303\274gt")
+
+add_file () {
+   (
+   cd "$1" &&
+   shift &&
+   for name
+   do
+   echo "$name" >"$name" &&
+   git add "$name" &&
+   test_tick &&
+   # "git commit -m" would break MinGW, as Windows refuse 
to pass
+   # $test_encoding encoded parameter to git.
+   echo "Add $name ($added $name)" | iconv -f utf-8 -t 
$test_encoding |
+   git -c "i18n.commitEncoding=$test_encoding" commit -F -
+   done >/dev/null &&
+   git rev-parse --short --verify HEAD
+   )
+}
+
+commit_file () {
+   test_tick 

[PATCH v9 0/8] submodule show inline diff

[Jacob Keller]

From: Jacob Keller 

More suggestions from Junio and a few changes to support submodule name
lookup. Hopefully we're getting close to the goal!

interdiff between v8 and current:
diff --git c/builtin/rev-list.c w/builtin/rev-list.c
index 21cde8dd6b31..8479f6ed28aa 100644
--- c/builtin/rev-list.c
+++ w/builtin/rev-list.c
@@ -129,29 +129,29 @@ static void show_commit(struct commit *commit, void *data)
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.)
-   */
+* 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 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)
diff --git c/cache.h w/cache.h
index da9f0be67d7b..70428e92d7ed 100644
--- c/cache.h
+++ w/cache.h
@@ -953,24 +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)
 
-extern const struct object_id empty_tree_oid;
 
 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 c/diff.h w/diff.h
index 192c0eedd0ff..ec76a90522ea 100644
--- c/diff.h
+++ w/diff.h
@@ -112,7 +112,7 @@ enum diff_words_type {
 enum diff_submodule_format {
DIFF_SUBMODULE_SHORT = 0,
DIFF_SUBMODULE_LOG,
-   DIFF_SUBMODULE_INLINE_DIFF,
+   DIFF_SUBMODULE_INLINE_DIFF
 };
 
 struct diff_options {
diff --git 

Re: [PATCH v8 4/8] submodule: allow add_submodule_odb to work even if path is not checked out

[Jacob Keller]

On Fri, Aug 19, 2016 at 3:26 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> Is there a way to do this lookup? I couldn't find it.
>
> Perhaps submodule_from_path(), that is used to implement "git
> submodule--helper name $path"?

Yep, thanks. I also added a test to show that the use of name works.

Regards,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[ANNOUNCE] Git v2.10.0-rc1

[Junio C Hamano]

A release candidate Git v2.10.0-rc1 is now available for testing
at the usual places.  It is comprised of 611 non-merge commits
since v2.9.0, contributed by 68 people, 20 of which are new faces.

Relative to v2.10-0-rc0 (the preview), there is a last-minute merge
of Linus's "Let's show longer fingerprint in the GPG output" patch
(which was a bit scary but I made sure push-certificate codepath
would not be negatively affected), among other small topics.

The tarballs are found at:

https://www.kernel.org/pub/software/scm/git/testing/

The following public repositories all have a copy of the
'v2.10.0-rc1' tag and the 'master' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

New contributors whose contributions weren't in v2.9.0 are as follows.
Welcome to the Git development community!

  Alexander Hirsch, Andreas Brauchli, Andrew Oakley, Antoine
  Queru, Christopher Layne, Dave Nicolson, Ed Maste, Heiko Becker,
  Ingo Brückl, Jonathan Tan, Jordan DE GEA, Josef Kufner, Keith
  McGuigan, Kevin Willford, LE Manh Cuong, Michael Stahl, Parker
  Moore, Peter Colberg, Tom Russello, and William Duclot.

Returning contributors who helped this release are as follows.
Thanks for your continued support.

  Alex Henrie, Alfred Perlstein, Armin Kunaschik, brian m. carlson,
  Charles Bailey, Chris Packham, Christian Couder, David A. Greene,
  David Aguilar, David Kastrup, David Turner, Edward Thomson, Elia
  Pinto, Eric Sunshine, Eric Wong, Heiko Voigt, Jacob Keller,
  Jeff King, Joey Hess, Johannes Schindelin, Johannes Sixt,
  John Keeping, Jonathan Nieder, Josh Triplett, Junio C Hamano,
  Lars Schneider, Lars Vogel, Linus Torvalds, Lukas Fleischer,
  Matthieu Moy, Mehul Jain, Michael Haggerty, Michael J Gruber,
  Mike Hommey, Nguyễn Thái Ngọc Duy, Nicolas Pitre, Orgad
  Shaneh, Patrick Steinhardt, Pranit Bauva, Ramsay Jones, René
  Scharfe, Ronald Wampler, Stefan Beller, SZEDER Gábor, Thomas
  Braun, Torsten Bögershausen, Vasco Almeida, and Ville Skyttä.



Git 2.10 Release Notes (draft)
==

Backward compatibility notes


Updates since v2.9
--

UI, Workflows & Features

 * "git pull --rebase --verify-signature" learned to warn the user
   that "--verify-signature" is a no-op when rebasing.

 * An upstream project can make a recommendation to shallowly clone
   some submodules in the .gitmodules file it ships.

 * "git worktree add" learned that '-' can be used as a short-hand for
   "@{-1}", the previous branch.

 * Update the funcname definition to support css files.

 * The completion script (in contrib/) learned to complete "git
   status" options.

 * Messages that are generated by auto gc during "git push" on the
   receiving end are now passed back to the sending end in such a way
   that they are shown with "remote: " prefix to avoid confusing the
   users.

 * "git add -i/-p" learned to honor diff.compactionHeuristic
   experimental knob, so that the user can work on the same hunk split
   as "git diff" output.

 * "upload-pack" allows a custom "git pack-objects" replacement when
   responding to "fetch/clone" via the uploadpack.packObjectsHook.
   (merge b738396 jk/upload-pack-hook later to maint).

 * Teach format-patch and mailsplit (hence "am") how a line that
   happens to begin with "From " in the e-mail message is quoted with
   ">", so that these lines can be restored to their original shape.
   (merge d9925d1 ew/mboxrd-format-am later to maint).

 * "git repack" learned the "--keep-unreachable" option, which sends
   loose unreachable objects to a pack instead of leaving them loose.
   This helps heuristics based on the number of loose objects
   (e.g. "gc --auto").
   (merge e26a8c4 jk/repack-keep-unreachable later to maint).

 * "log --graph --format=" learned that "%>|(N)" specifies the width
   relative to the terminal's left edge, not relative to the area to
   draw text that is to the right of the ancestry-graph section.  It
   also now accepts negative N that means the column limit is relative
   to the right border.

 * A careless invocation of "git send-email directory/" after editing
   0001-change.patch with an editor often ends up sending both
   0001-change.patch and its backup file, 0001-change.patch~, causing
   embarrassment and a minor confusion.  Detect such an input and
   offer to skip the backup files when sending the patches out.
   (merge 531220b jc/send-email-skip-backup later to maint).

 * "git submodule update" that drives many "git clone" could
   eventually hit flaky servers/network conditions on one of the
   submodules; the command learned to retry the attempt.

 * The output coloring scheme 

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Eric Wong]

Johannes Schindelin  wrote:
> On Thu, 18 Aug 2016, Eric Wong wrote:
> > Johannes Schindelin  wrote:
> >
> > > Old dogs claim the mail list-approach works for them. Nope. Doesn't.
> > > Else you would not have written all those custom scripts.
> > 
> > git and cogito started as a bunch of custom scripts, too.
> 
> The difference is that neither git nor cogito were opinionated. Those
> custom scripts are. They are for one particular workflow, with one
> particular mail client, with a strong bias to a Unix-y environment.
> 
> I work really hard to make Git for Windows as easy and fun to use as
> possible. I just wish that we were working together to make it as easy and
> fun to contribute to Git, too.

I guess this is a fundamental difference between *nix and
Windows culture.

I enjoy using and contributing to git because it interacts well
with generic tools.  *nix kernels are optimized for this with
decent (not great)[*] process spawning and IPC performance.

I know Windows users have major performance problems with
shell scripts; but they are also largely helpless to improve
Windows kernel performance.

So, I guess monolithic tools became popular on Windows, instead.

> > I see a choice of mail client as no different than a choice of
> > text editor.  Neither my mail client or text editor is heavily
> > customized.  The key feature I rely on from both tools is piping
> > data to external commands.
> 
> There you go. That key feature seems to be unavailable in the most
> wide-spread email client: Outlook. So by not restricting the choice you
> should make it possible to use that mail client, too, right?
> 
> We do not even have a section on Outlook in our SubmittingPatches.
> 
> Okay, if not the most popular mail client, then web mail? Nope, nope,
> nope. No piping *at all* to external commands from there.
> 
> So you basically slam the door shut on the vast majority of email users.

Users have a choice to use a more scriptable mail client
(but I guess the OS nudges users towards monolithic tools)

It's unfortunate the world is so full of proprietary things;
but I think it's our responsibility as Free Software developers
to encourage the use of Free (or "Open Source") tools which
users can control.

> That is not leaving much choice to the users in my book.

Users of alpine, gnus, mutt, sylpheed, thunderbird, kmail,
roundcube, squirelmail, etc. can all download the source, hack,
fix and customize things.  It's easier with smaller software,
of course:  git-send-email does not even require learning
the build process or separate download.



> > Users ought to be able to pick, choose, and replace tools as
> > they wish as long as an interchange format remains stable
> > and widely-supported.
> 
> Right. Let's talk about the interchange format called mails, for the data
> called patches. Is it stable and widely-supported?
> 
> Can users really pick and choose the tools they like most to send patches
> to the Git project? Like, the Outlook client? Or the GMail client?

Personally, I don't mind patches as MIME attachments if that
avoids corruption, MIME seems well-supported at this point.
It's not my call, though.  But as Stephan pointed, Linus
does it, too.



[*] Guess what: I have performance problems with fork/execve on
Linux, too.  However, Linux developers already provide
mechanisms to improve spawn performance (CLONE_VFORK and
vfork(2)); so the next step is to get userspace like dash,
make, perl, etc to support these.

glibc 2.24 was just released with an improved posix_spawn
for Linux (using CLONE_VFORK), so that's a step forward and
might make sharing code with Windows easier, too.

It's not a high priority for me at the moment, but I intend
to get everything in my toolset which relies on fork+execve
to use posix_spawn or vfork+execve instead.  I have the
source to all of these, so at least I can do something
about it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Eric Wong]

Stefan Beller  wrote:
> Maybe we should invent a patch format that copes with broken whitespace?

No redundant new formats, please.  MIME attachments are already
widely-supported and fine by me.  But it's not my call for git.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] diff-highlight: add some tests.

[Junio C Hamano]

Jeff King  writes:

> For that matter, I'm not sure that:
>
>   cat >a <<-\EOF &&
>   aaa
>   bbb
>   ccc
>   EOF
>
>   cat >b <<-\EOF &&
>   aaa
>   0bb
>   ccc
>   EOF
>
>   dh_test a b <<\EOF
>   aaa
>   -${CW}b${CR}bb
>   +${CW}0${CR}bb
>   EOF
>
> isn't more readable, too. It's more lines, certainly, but it makes it
> very easy to see what the input files look like, rather than cramming
> "\n" into the middle of a string (the existing code does make the diff
> easy to see for _this_ case, because the pre- and post-image line up
> vertically, but that is only the case for pure transliterations like
> this).

Yeah, the simplicity and obviousness certainly is very tempting.

With something like

CW=$(printf "\033[7m")  # white
CR=$(printf "\033[27m") # reset

upfront, the test helper does not even need to worry about feeding a
random string through printf as if it is a format string.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 4/8] submodule: allow add_submodule_odb to work even if path is not checked out

[Junio C Hamano]

Jacob Keller  writes:

> Is there a way to do this lookup? I couldn't find it.

Perhaps submodule_from_path(), that is used to implement "git
submodule--helper name $path"?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jacob Keller]

On Fri, Aug 19, 2016 at 2:52 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> diff --git a/diff.h b/diff.h
>> index ea5aba668eaa..192c0eedd0ff 100644
>> --- a/diff.h
>> +++ b/diff.h
>> @@ -112,6 +112,7 @@ enum diff_words_type {
>>  enum diff_submodule_format {
>>   DIFF_SUBMODULE_SHORT = 0,
>>   DIFF_SUBMODULE_LOG,
>> + DIFF_SUBMODULE_INLINE_DIFF,
>
> Same trailing comma.
>
>>  };
>>
>>  struct diff_options {
>> diff --git a/submodule.c b/submodule.c
>> index 7108b4786bc1..cecd3cd98de4 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -435,6 +435,68 @@ void show_submodule_summary(FILE *f, const char *path,
>>   clear_commit_marks(right, ~0);
>>  }
>>
>> +void show_submodule_inline_diff(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 struct diff_options *o)
>> +{
>> + const struct object_id *old = _tree_oid, *new = _tree_oid;
>> + struct commit *left = NULL, *right = NULL;
>> + struct strbuf submodule_dir = STRBUF_INIT;
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> +
>> + show_submodule_header(f, path, line_prefix, one, two, dirty_submodule,
>> +   meta, reset, , );
>> +
>> + /* We need a valid left and right commit to display a difference */
>> + if (!(left || is_null_oid(one)) ||
>> + !(right || is_null_oid(two)))
>> + goto done;
>> +
>> + if (left)
>> + old = one;
>> + if (right)
>> + new = two;
>> +
>> + fflush(f);
>> + cp.git_cmd = 1;
>> + cp.dir = path;
>> + cp.out = dup(fileno(f));
>> + cp.no_stdin = 1;
>> +
>> + /* TODO: other options may need to be passed here. */
>> + argv_array_pushl(, "diff");
>
> I think you meant argv_array_push() here.  Or ", NULL" at the end if
> you anticipate you would grow more args after "diff" later and keep
> using pushl().

I had added an argument at one point and accidentally forgot to
convert back to push(). Oops!

Regards,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v13 05/13] t6030: explicitly test for bisection cleanup

[Pranit Bauva]

Add test to explicitly check that 'git bisect reset' is working as
expected. This is already covered implicitly by the test suite.

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

---
I faced this problem while converting `bisect_clean_state` and the tests
where showing breakages but it wasn't clear as to where exactly are they
breaking. This will patch  will help in that. Also I tested the test
coverage of the test suite before this patch and it covers this (I did
this by purposely changing names of files in git-bisect.sh and running
the test suite).
---
 t/t6030-bisect-porcelain.sh | 17 +
 1 file changed, 17 insertions(+)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 5e5370f..18e7998 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs 
in any order' '
test_cmp expected actual
 '
 
+test_expect_success 'git bisect reset cleans bisection state properly' '
+   git bisect reset &&
+   git bisect start &&
+   git bisect good $HASH1 &&
+   git bisect bad $HASH4 &&
+   git bisect reset &&
+   test -z "$(git for-each-ref "refs/bisect/*")" &&
+   test_path_is_missing "$GIT_DIR/BISECT_EXPECTED_REV" &&
+   test_path_is_missing "$GIT_DIR/BISECT_ANCESTORS_OK" &&
+   test_path_is_missing "$GIT_DIR/BISECT_LOG" &&
+   test_path_is_missing "$GIT_DIR/BISECT_RUN" &&
+   test_path_is_missing "$GIT_DIR/BISECT_TERMS" &&
+   test_path_is_missing "$GIT_DIR/head-name" &&
+   test_path_is_missing "$GIT_DIR/BISECT_HEAD" &&
+   test_path_is_missing "$GIT_DIR/BISECT_START"
+'
+
 test_done

--
https://github.com/git/git/pull/281
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 7/8] cache: add empty_tree_oid object

[Jacob Keller]

On Fri, Aug 19, 2016 at 2:14 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Jacob Keller  writes:
>>
>>> Is there a reason for that? I've found that .field = value is safer
>>> because it ensures that you don't end up initializing the wrong
>>> values? Or is it a compatibility thing?
>>
>> Yes.
>
> That was a bit too terse.  The answers to all three questions are
> "Yes", "Yes", and "Yes".
>
> Together with enum ... = { A, B, C, }; we may want to consider not
> worrying about ancient compilers at some point, and it might be this
> year or next year, but I do not think we want to do that as part of
> this series.

Thanks, it makes sense. It is hard to remember when some feature got added.

Regards,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 4/8] submodule: allow add_submodule_odb to work even if path is not checked out

[Jacob Keller]

On Fri, Aug 19, 2016 at 2:11 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> submodule. I think we already have the complete path. Or is the name
>> *not* equivalent to the path?
>
> A submodule that is bound to top-level at "path" originally gets
> "path" as its name.  If you move it elsewhere, you do not want it to
> lose its identity (and its place in .git/modules/* of the
> top-level).  so a submodule whose name is "path" can reside in the
> new place after such a move.
>

Is there a way to do this lookup? I couldn't find it.

>> There was no empty line in the place I copied from.
>
> Is that "because I copied from a source that is mistaken, I refuse
> to make it right"?  Or just an explanation why there is a mistake?
> Or something else (like "we should update the original one while we
> are at it as a pure clean-up")?

That was an explanation for "I didn't understand that was a mistake"
and a "if we fix this we might want to fix them also to avoid this
same problem in the future".

>
>> If we put them in test_expect_success setup they aren't.
>
> Yes, that is why I said they are unnecessary.  Let's minimize the
> amount of random code that sits outside the control of the test
> framework (i.e. test_expect_{success,failure}).
>

Yes I agree.

Thanks,
Jake

> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jacob Keller  writes:

> diff --git a/diff.h b/diff.h
> index ea5aba668eaa..192c0eedd0ff 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -112,6 +112,7 @@ enum diff_words_type {
>  enum diff_submodule_format {
>   DIFF_SUBMODULE_SHORT = 0,
>   DIFF_SUBMODULE_LOG,
> + DIFF_SUBMODULE_INLINE_DIFF,

Same trailing comma.

>  };
>  
>  struct diff_options {
> diff --git a/submodule.c b/submodule.c
> index 7108b4786bc1..cecd3cd98de4 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -435,6 +435,68 @@ void show_submodule_summary(FILE *f, const char *path,
>   clear_commit_marks(right, ~0);
>  }
>  
> +void show_submodule_inline_diff(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 struct diff_options *o)
> +{
> + const struct object_id *old = _tree_oid, *new = _tree_oid;
> + struct commit *left = NULL, *right = NULL;
> + struct strbuf submodule_dir = STRBUF_INIT;
> + struct child_process cp = CHILD_PROCESS_INIT;
> +
> + show_submodule_header(f, path, line_prefix, one, two, dirty_submodule,
> +   meta, reset, , );
> +
> + /* We need a valid left and right commit to display a difference */
> + if (!(left || is_null_oid(one)) ||
> + !(right || is_null_oid(two)))
> + goto done;
> +
> + if (left)
> + old = one;
> + if (right)
> + new = two;
> +
> + fflush(f);
> + cp.git_cmd = 1;
> + cp.dir = path;
> + cp.out = dup(fileno(f));
> + cp.no_stdin = 1;
> +
> + /* TODO: other options may need to be passed here. */
> + argv_array_pushl(, "diff");

I think you meant argv_array_push() here.  Or ", NULL" at the end if
you anticipate you would grow more args after "diff" later and keep
using pushl().
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] for-each-ref: add %(upstream:gone) to mark missing refs

[Øystein Walle]

git branch -vv will show "gone" next to a remote tracking branch if it
does not exist. for-each-ref is suitable for parsing but had no way of
showing this information.

This introduces "%(upstream:gone)" to display "gone" in the formatted
output if the ref does not exist or an empty string otherwise, analogous
to git branch -vv.

Signed-off-by: Øystein Walle 
---
 Documentation/git-for-each-ref.txt |  5 +++--
 ref-filter.c   | 10 +-
 t/t6300-for-each-ref.sh| 12 
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index f57e69b..039a86b 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -114,8 +114,9 @@ upstream::
`refname` above.  Additionally respects `:track` to show
"[ahead N, behind M]" and `:trackshort` to show the terse
version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
-   or "=" (in sync).  Has no effect if the ref does not have
-   tracking information associated with it.
+   or "=" (in sync) and `:gone` to show "gone" if the remote ref
+   does not exist, or an empty string if it does. Has no effect if
+   the ref does not have tracking information associated with it.
 
 push::
The name of a local ref which represents the `@{push}` location
diff --git a/ref-filter.c b/ref-filter.c
index bc551a7..5402052 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -37,7 +37,7 @@ static struct used_atom {
union {
char color[COLOR_MAXLEN];
struct align align;
-   enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
+   enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_GONE }
remote_ref;
struct {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB } option;
@@ -67,6 +67,8 @@ static void remote_ref_atom_parser(struct used_atom *atom, 
const char *arg)
atom->u.remote_ref = RR_TRACK;
else if (!strcmp(arg, "trackshort"))
atom->u.remote_ref = RR_TRACKSHORT;
+   else if (!strcmp(arg, "gone"))
+   atom->u.remote_ref = RR_GONE;
else
die(_("unrecognized format: %%(%s)"), atom->name);
 }
@@ -923,6 +925,12 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
*s = ">";
else
*s = "<>";
+   } else if (atom->u.remote_ref == RR_GONE) {
+   const char *upstream;
+   if (stat_tracking_info(branch, _ours, _theirs, 
) < 0)
+   *s = "gone";
+   else
+   *s = "";
} else /* RR_NORMAL */
*s = refname;
 }
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 19a2823..1fc5d1d 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -383,6 +383,18 @@ test_expect_success 'Check that :track[short] works when 
upstream is invalid' '
test_cmp expected actual
 '
 
+test_expect_success 'Check that :gone produces expected results' '
+   cat >expected <<-\EOF &&
+gone
+   EOF
+   test_when_finished "git config branch.master.merge refs/heads/master" &&
+   git config branch.master.merge refs/heads/does-not-exist &&
+   git for-each-ref \
+   --format="%(upstream:gone)" \
+   refs/heads >actual &&
+   test_cmp expected actual
+'
+
 test_expect_success 'Check for invalid refname format' '
test_must_fail git for-each-ref --format="%(refname:INVALID)"
 '
-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v13 10/13] bisect--helper: `check_and_set_terms` shell function in C

[Pranit Bauva]

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

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

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

Also introduce bisect_terms_reset() to empty the contents of `term_good`
and `term_bad` of `struct bisect_terms`.

Also introduce set_terms() to copy the `term_good` and `term_bad` into
`struct bisect_terms` and write it out to the file BISECT_TERMS.

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

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 5364960..450426c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,6 +20,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-clean-state"),
N_("git bisect--helper --bisect-reset []"),
N_("git bisect--helper --bisect-write
 []"),
+   N_("git bisect--helper --bisect-check-and-set-terms  
 "),
NULL
 };
 
@@ -40,6 +41,12 @@ static void bisect_terms_release(struct bisect_terms *terms)
strbuf_release(>term_bad);
 }
 
+static void bisect_terms_reset(struct bisect_terms *term)
+{
+   strbuf_reset(>term_good);
+   strbuf_reset(>term_bad);
+}
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -213,6 +220,39 @@ static int bisect_write(const char *state, const char *rev,
return 0;
 }
 
+static int set_terms(struct bisect_terms *terms, const char *bad,
+const char *good)
+{
+   bisect_terms_reset(terms);
+   strbuf_addstr(>term_good, good);
+   strbuf_addstr(>term_bad, bad);
+   return write_terms(terms->term_bad.buf, terms->term_good.buf);
+}
+
+static int check_and_set_terms(struct bisect_terms *terms, const char *cmd)
+{
+   int has_term_file = !is_empty_or_missing_file(git_path_bisect_terms());
+
+   if (one_of(cmd, "skip", "start", "terms", NULL))
+   return 0;
+
+   if (has_term_file &&
+   strcmp(cmd, terms->term_bad.buf) &&
+   strcmp(cmd, terms->term_good.buf))
+   return error(_("Invalid command: you're currently in a "
+   "%s/%s bisect"), terms->term_bad.buf,
+   terms->term_good.buf);
+
+   if (!has_term_file) {
+   if (one_of(cmd, "bad", "good", NULL))
+   return set_terms(terms, "bad", "good");
+   if (one_of(cmd, "new", "old", NULL))
+   return set_terms(terms, "new", "old");
+   }
+
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -221,7 +261,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
BISECT_CLEAN_STATE,
BISECT_RESET,
CHECK_EXPECTED_REVS,
-   BISECT_WRITE
+   BISECT_WRITE,
+   CHECK_AND_SET_TERMS
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -237,6 +278,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("check for expected revs"), CHECK_EXPECTED_REVS),
OPT_CMDMODE(0, "bisect-write", ,
 N_("write out the bisection state in BISECT_LOG"), 
BISECT_WRITE),
+   OPT_CMDMODE(0, "check-and-set-terms", ,
+N_("check and set terms in a bisection state"), 
CHECK_AND_SET_TERMS),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -280,6 +323,13 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
strbuf_addstr(_bad, argv[3]);
res = bisect_write(argv[0], argv[1], , nolog);
break;
+   case CHECK_AND_SET_TERMS:
+   if (argc != 3)
+   

Re: [PATCH v2 0/3] diff-highlight: add support for git log --graph output.

[Jeff King]

On Fri, Aug 19, 2016 at 09:19:44PM +, Eric Wong wrote:

> > I've rebased my changes. Unfortunately, having 3 commits made this somewhat
> > tedious. I also find it weird that my new patch set makes it difficult to 
> > see
> > what changes I've made from my first set. What's the standard workflow here?
> 
> I check out a new branch with the same base as the previous series
> and "git diff previous current"
> 
> (without git, I'd be using interdiff from the patchutils Debian package)
> 
> Sometimes I will rebase against both old+new against Junio's master
> to avoid/reduce conflicts.

You might also try Thomas Rast's topic-branch diff:

  https://github.com/trast/tbdiff

It gives better patch-by-patch differences. And it handles the case that
the topics have different bases (which is handy if you've rebased, or if
Junio happened to apply on a different base than you did).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] diff-highlight: add support for git log --graph output.

[Eric Wong]

Brian Henderson  wrote:
> On Wed, Aug 10, 2016 at 08:56:35AM +, Eric Wong wrote:
> > "local" is not a portable construct.  It's common for
> > Debian/Ubuntu systems to use dash as /bin/sh instead of bash;
> > (dash is faster, and mostly sticks to POSIX)
> > 
> > The "devscripts" package in Debian/Ubuntu-based systems has a
> > handy "checkbashisms" tool for checking portability of shell
> > scripts.
> 
> checkbashisms didn't output anything, and I found other instances of
> local in some tests. but I removed anyway.

Ah, I guess "checkbashisms --posix" is required nowadays
since Debian policy deviated from POSIX, here
(we don't blindly follow POSIX, either).

Anyways, some people still care about ksh93 as of a few months
ago; so I think avoiding "local" is preferable:

  https://public-inbox.org/git/?q=ksh93:..20160801

I think all of our other "local" uses are limited
to bash-specific parts: bash completion, mingw tests



> I've rebased my changes. Unfortunately, having 3 commits made this somewhat
> tedious. I also find it weird that my new patch set makes it difficult to see
> what changes I've made from my first set. What's the standard workflow here?

I check out a new branch with the same base as the previous series
and "git diff previous current"

(without git, I'd be using interdiff from the patchutils Debian package)

Sometimes I will rebase against both old+new against Junio's master
to avoid/reduce conflicts.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v13 04/13] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

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

Using `--bisect-clean-state` subcommand is a measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired but its implementation  will
be called by bisect_reset() and bisect_start().

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 bisect.c | 43 +++
 bisect.h |  2 ++
 builtin/bisect--helper.c | 14 +-
 git-bisect.sh| 26 +++---
 4 files changed, 61 insertions(+), 24 deletions(-)

diff --git a/bisect.c b/bisect.c
index 6f512c2..45d598d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -430,6 +430,12 @@ static int read_bisect_refs(void)
 
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
+static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
+static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
+static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
+static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
+static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+static GIT_PATH_FUNC(git_path_head_name, "head-name")
 
 static void read_bisect_paths(struct argv_array *array)
 {
@@ -1040,3 +1046,40 @@ int estimate_bisect_steps(int all)
 
return (e < 3 * x) ? n : n - 1;
 }
+
+static int mark_for_removal(const char *refname, const struct object_id *oid,
+   int flag, void *cb_data)
+{
+   struct string_list *refs = cb_data;
+   char *ref = xstrfmt("refs/bisect%s", refname);
+   string_list_append(refs, ref);
+   return 0;
+}
+
+int bisect_clean_state(void)
+{
+   int result = 0;
+
+   /* There may be some refs packed during bisection */
+   struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
+   for_each_ref_in("refs/bisect", mark_for_removal, (void *) 
_for_removal);
+   string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
+   result = delete_refs(_for_removal, REF_NODEREF);
+   refs_for_removal.strdup_strings = 1;
+   string_list_clear(_for_removal, 0);
+   unlink_or_warn(git_path_bisect_expected_rev());
+   unlink_or_warn(git_path_bisect_ancestors_ok());
+   unlink_or_warn(git_path_bisect_log());
+   unlink_or_warn(git_path_bisect_names());
+   unlink_or_warn(git_path_bisect_run());
+   unlink_or_warn(git_path_bisect_terms());
+   /* Cleanup head-name if it got left by an old version of git-bisect */
+   unlink_or_warn(git_path_head_name());
+   /*
+* Cleanup BISECT_START last to support the --no-checkout option
+* introduced in the commit 4796e823a.
+*/
+   unlink_or_warn(git_path_bisect_start());
+
+   return result;
+}
diff --git a/bisect.h b/bisect.h
index acd12ef..0ae63d4 100644
--- a/bisect.h
+++ b/bisect.h
@@ -28,4 +28,6 @@ extern int estimate_bisect_steps(int all);
 
 extern void read_bisect_terms(const char **bad, const char **good);
 
+extern int bisect_clean_state(void);
+
 #endif
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 30e1031..e50934c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -5,10 +5,15 @@
 #include "refs.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
+static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
+static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
+static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms  "),
+   N_("git bisect--helper --bisect-clean-state"),
NULL
 };
 
@@ -83,7 +88,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 {
enum {
NEXT_ALL = 1,
-   WRITE_TERMS
+   WRITE_TERMS,
+   BISECT_CLEAN_STATE
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -91,6 +97,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("perform 'git bisect next'"), NEXT_ALL),
OPT_CMDMODE(0, "write-terms", ,
 N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
+   OPT_CMDMODE(0, "bisect-clean-state", ,
+N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of 

Re: [PATCH v8 7/8] cache: add empty_tree_oid object

[Junio C Hamano]

Junio C Hamano  writes:

> Jacob Keller  writes:
>
>> Is there a reason for that? I've found that .field = value is safer
>> because it ensures that you don't end up initializing the wrong
>> values? Or is it a compatibility thing?
>
> Yes.

That was a bit too terse.  The answers to all three questions are
"Yes", "Yes", and "Yes".

Together with enum ... = { A, B, C, }; we may want to consider not
worrying about ancient compilers at some point, and it might be this
year or next year, but I do not think we want to do that as part of
this series.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v13 06/13] wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()

[Pranit Bauva]

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

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

diff --git a/builtin/am.c b/builtin/am.c
index 739b34d..9e1e9d6 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -30,22 +30,6 @@
 #include "mailinfo.h"
 
 /**
- * Returns 1 if the file is empty or does not exist, 0 otherwise.
- */
-static int is_empty_file(const char *filename)
-{
-   struct stat st;
-
-   if (stat(filename, ) < 0) {
-   if (errno == ENOENT)
-   return 1;
-   die_errno(_("could not stat %s"), filename);
-   }
-
-   return !st.st_size;
-}
-
-/**
  * Returns the length of the first line of msg.
  */
 static int linelen(const char *msg)
@@ -1324,7 +1308,7 @@ static int parse_mail(struct am_state *state, const char 
*mail)
goto finish;
}
 
-   if (is_empty_file(am_path(state, "patch"))) {
+   if (is_empty_or_missing_file(am_path(state, "patch"))) {
printf_ln(_("Patch is empty. Was it split wrong?"));
die_user_resolve(state);
}
@@ -1896,7 +1880,7 @@ static void am_run(struct am_state *state, int resume)
resume = 0;
}
 
-   if (!is_empty_file(am_path(state, "rewritten"))) {
+   if (!is_empty_or_missing_file(am_path(state, "rewritten"))) {
assert(state->rebasing);
copy_notes_for_rebase(state);
run_post_rewrite_hook(state);
diff --git a/cache.h b/cache.h
index b780a91..49f214b 100644
--- a/cache.h
+++ b/cache.h
@@ -1916,4 +1916,7 @@ void sleep_millisec(int millisec);
  */
 void safe_create_dir(const char *dir, int share);
 
+/* Return 1 if the file is empty or does not exists, 0 otherwise. */
+extern int is_empty_or_missing_file(const char *filename);
+
 #endif /* CACHE_H */
diff --git a/wrapper.c b/wrapper.c
index e7f1979..78f6431 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -679,3 +679,16 @@ void sleep_millisec(int millisec)
 {
poll(NULL, 0, millisec);
 }
+
+int is_empty_or_missing_file(const char *filename)
+{
+   struct stat st;
+
+   if (stat(filename, ) < 0) {
+   if (errno == ENOENT)
+   return 1;
+   die_errno(_("could not stat %s"), filename);
+   }
+
+   return !st.st_size;
+}

--
https://github.com/git/git/pull/281
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 4/8] submodule: allow add_submodule_odb to work even if path is not checked out

[Junio C Hamano]

Jacob Keller  writes:

> submodule. I think we already have the complete path. Or is the name
> *not* equivalent to the path?

A submodule that is bound to top-level at "path" originally gets
"path" as its name.  If you move it elsewhere, you do not want it to
lose its identity (and its place in .git/modules/* of the
top-level).  so a submodule whose name is "path" can reside in the
new place after such a move.

> There was no empty line in the place I copied from.

Is that "because I copied from a source that is mistaken, I refuse
to make it right"?  Or just an explanation why there is a mistake?
Or something else (like "we should update the original one while we
are at it as a pure clean-up")?

> If we put them in test_expect_success setup they aren't.

Yes, that is why I said they are unnecessary.  Let's minimize the
amount of random code that sits outside the control of the test
framework (i.e. test_expect_{success,failure}).

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v13 03/13] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

Reimplement the `write_terms` shell function in C and add a `write-terms`
subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
remove the subcommand `--check-term-format` as it can now be called from
inside the function write_terms() C implementation.

Also `|| exit` is added when calling write-terms subcommand from
git-bisect.sh so as to exit whenever there is an error.

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

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

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index a47f1f2..30e1031 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,9 +4,11 @@
 #include "bisect.h"
 #include "refs.h"
 
+static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
-   N_("git bisect--helper --check-term-format  "),
+   N_("git bisect--helper --write-terms  "),
NULL
 };
 
@@ -57,18 +59,38 @@ static int check_term_format(const char *term, const char 
*orig_term)
return 0;
 }
 
+static int write_terms(const char *bad, const char *good)
+{
+   FILE *fp;
+   int res;
+
+   if (!strcmp(bad, good))
+   return error(_("please use two different terms"));
+
+   if (check_term_format(bad, "bad") || check_term_format(good, "good"))
+   return -1;
+
+   fp = fopen(git_path_bisect_terms(), "w");
+   if (!fp)
+   return error_errno(_("could not open the file BISECT_TERMS"));
+
+   res = fprintf(fp, "%s\n%s\n", bad, good);
+   res |= fclose(fp);
+   return (res < 0) ? -1 : 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
NEXT_ALL = 1,
-   CHECK_TERM_FMT
+   WRITE_TERMS
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
OPT_CMDMODE(0, "next-all", ,
 N_("perform 'git bisect next'"), NEXT_ALL),
-   OPT_CMDMODE(0, "check-term-format", ,
-N_("check format of the term"), CHECK_TERM_FMT),
+   OPT_CMDMODE(0, "write-terms", ,
+N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -83,10 +105,10 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
switch (cmdmode) {
case NEXT_ALL:
return bisect_next_all(prefix, no_checkout);
-   case CHECK_TERM_FMT:
+   case WRITE_TERMS:
if (argc != 2)
-   die(_("--check-term-format requires two arguments"));
-   return check_term_format(argv[0], argv[1]);
+   die(_("--write-terms requires two arguments"));
+   return write_terms(argv[0], argv[1]);
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index a727c59..9ef6cb8 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -209,7 +209,7 @@ bisect_start() {
eval "$eval true" &&
if test $must_write_terms -eq 1
then
-   write_terms "$TERM_BAD" "$TERM_GOOD"
+   git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
fi &&
echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
#
@@ -557,18 +557,6 @@ get_terms () {
fi
 }
 
-write_terms () {
-   TERM_BAD=$1
-   TERM_GOOD=$2
-   if test "$TERM_BAD" = "$TERM_GOOD"
-   then
-   die "$(gettext "please use two different terms")"
-   fi
-   git bisect--helper --check-term-format "$TERM_BAD" bad || exit
-   git bisect--helper --check-term-format "$TERM_GOOD" good || exit
-   printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
-}
-
 check_and_set_terms () {
cmd="$1"
case "$cmd" in
@@ -582,13 +570,17 @@ check_and_set_terms () {
bad|good)
if ! test -s "$GIT_DIR/BISECT_TERMS"
then
-   write_terms bad good
+   TERM_BAD=bad
+   TERM_GOOD=good
+   git 

Re: [PATCH] diff-highlight: add some tests.

[Junio C Hamano]

Brian Henderson  writes:

> +# dh_test is a test helper function which takes 1) some file data, 2) some
> +# change of the file data, creates a diff and commit of the changes and 
> passes
> +# that through diff-highlight.
> +# The optional 3rd parameter is the expected output of diff-highlight minus 
> the
> +# diff/commit header. This parameter is given directly to printf as the 
> format
> +# string (in order to properly handle ascii escape codes; CW, CR), so any '%'
> +# need to be doubled to protect it.

My mistake.  All three are given directly to printf as the format
string, so it is misleading to single out $3 when warning the
callers about '%'.

Sorry about that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] diff-highlight: add some tests.

[Jeff King]

On Fri, Aug 19, 2016 at 01:44:28PM -0700, Junio C Hamano wrote:

> Brian Henderson  writes:
> 
> > Junio, how does this look?
> > ...
> > +# dh_test is a test helper function which takes 1) some file data, 2) some
> > +# change of the file data, creates a diff and commit of the changes and 
> > passes
> > +# that through diff-highlight.
> > +# The optional 3rd parameter is the expected output of diff-highlight 
> > minus the
> > +# diff/commit header. This parameter is given directly to printf as the 
> > format
> > +# string (in order to properly handle ascii escape codes; CW, CR), so any 
> > '%'
> > +# need to be doubled to protect it.
> > +# Don't include a 3rd parameter if diff-highlight is supposed to leave the
> > +# input unmodified.
> > +# For convienence, the 3rd parameter can begin with a newline which will be
> > +# stripped.
> 
> You seem to be stripping any and all empty lines with "perl -pe"; I
> am not sure if that is sensible.
> 
> I really do not see the point of being able to spell
> 
> "
> aaa
> bbb
> "
> 
> when you can perfectly well read
> 
> "aaa
> bbb"
> 
> or even "aaa\nbbb\n" for that matter.  I personally do not think the
> difference is worth the cost of an extra invocation of Perl, but we
> already saw how stubborn you are, so there is no point spending my
> time on trying to convince you further.  Assuming that it is so
> precious that the input can start with an extra blank line, what you
> wrote is a sensible implementation.

I didn't want to bikeshed, so I resisted saying so up until now, but I
actually think:

  dh_test \
"aaa\nbbb\nccc\n" \
"aaa\n0bb\nccc\n" \
<<-EOF
  aaa
  -${CW}b${CR}bb
  +${CW}0${CR}bb
  EOF

might before readable, if only because it lets you indent the content to
match the rest of the test content. For that matter, I'm not sure that:

  cat >a <<-\EOF &&
  aaa
  bbb
  ccc
  EOF

  cat >b <<-\EOF &&
  aaa
  0bb
  ccc
  EOF

  dh_test a b <<\EOF
  aaa
  -${CW}b${CR}bb
  +${CW}0${CR}bb
  EOF

isn't more readable, too. It's more lines, certainly, but it makes it
very easy to see what the input files look like, rather than cramming
"\n" into the middle of a string (the existing code does make the diff
easy to see for _this_ case, because the pre- and post-image line up
vertically, but that is only the case for pure transliterations like
this).

Just my two cents.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 7/8] cache: add empty_tree_oid object

[Jeff King]

On Fri, Aug 19, 2016 at 01:31:27PM -0700, Junio C Hamano wrote:

> Jacob Keller  writes:
> 
> > From: Jacob Keller 
> >
> > Add an empty_tree_oid object which can be used in place of
> > EMPTY_TREE_SHA1_BIN_LITERAL for code which is being converted to struct
> > object_id.
> 
> How widely do you envision the users of this symbol would be spread
> across the entire codebase?  I am debating myself if we need a
> singleton in-core copy like this (if we end up referring to it from
> everywhere), or we only need one level higher abstraction,
> e.g. "is_empty_tree_oid()" helper (in which case I do not think we
> even need a singleton; just imitate how is_empty_blob_sha1() is
> implemented).

I suspect we need more than just the "is_empty" query. At least for the
blob case, we do hashcpy() it into a struct (which should eventually
become oidcpy). The empty-tree case even more so, as we pass it to
random functions like lookup_tree().

Our EMPTY_TREE_SHA1_BIN_LITERAL effectively ends up as a singleton
in-core, too; it's just handled transparently by the compiler, since
it's a literal. This effectively gives us _two_ singletons. We could do:

  const struct object_id empty_blob_oid = {
  "\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 (empty_blob_oid.hash)

It's possible the use of an actual string literal lets the compiler do
more optimizations, but I'd doubt it matters in practice. Probably it is
just sticking that literal somewhere in BSS and filling in the pointer
to it.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] diff-highlight: add some tests.

[Eric Wong]

Brian Henderson  wrote:
> On Fri, Aug 19, 2016 at 11:10:55AM -0700, Junio C Hamano wrote:
> > 
> > > +# vim: set noet
> > 
> > We tend to avoid cluttering the source with editor specific insns
> > like this.
> 
> oops.
> 
> Anyone have any suggestions for project level vim settings?

vim defaults work fine for me on FreeBSD and Debian:

tabstop=8 shiftwidth=8 softtabstop=0 noexpandtab

Generally, we take after the Linux kernel:

https://kernel.org/doc/Documentation/CodingStyle

Which I'm happy about and largely agree with.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v13 08/13] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

[Pranit Bauva]

Reimplement `is_expected_rev` & `check_expected_revs` shell function in
C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
call it from git-bisect.sh .

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

Helped-by: Eric Sunshine 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 33 -
 git-bisect.sh| 20 ++--
 2 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 9aba094..711be75 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -123,13 +123,40 @@ static int bisect_reset(const char *commit)
return bisect_clean_state();
 }
 
+static int is_expected_rev(const char *expected_hex)
+{
+   struct strbuf actual_hex = STRBUF_INIT;
+   int res = 0;
+   if (strbuf_read_file(_hex, git_path_bisect_expected_rev(), 0) >= 
0) {
+   strbuf_trim(_hex);
+   res = !strcmp(actual_hex.buf, expected_hex);
+   }
+   strbuf_release(_hex);
+   return res;
+}
+
+static int check_expected_revs(const char **revs, int rev_nr)
+{
+   int i;
+
+   for (i = 0; i < rev_nr; i++) {
+   if (!is_expected_rev(revs[i])) {
+   unlink_or_warn(git_path_bisect_ancestors_ok());
+   unlink_or_warn(git_path_bisect_expected_rev());
+   return 0;
+   }
+   }
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
NEXT_ALL = 1,
WRITE_TERMS,
BISECT_CLEAN_STATE,
-   BISECT_RESET
+   BISECT_RESET,
+   CHECK_EXPECTED_REVS
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -141,6 +168,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
OPT_CMDMODE(0, "bisect-reset", ,
 N_("reset the bisection state"), BISECT_RESET),
+   OPT_CMDMODE(0, "check-expected-revs", ,
+N_("check for expected revs"), CHECK_EXPECTED_REVS),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -167,6 +196,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
if (argc > 1)
die(_("--bisect-reset requires either zero or one 
arguments"));
return bisect_reset(argc ? argv[0] : NULL);
+   case CHECK_EXPECTED_REVS:
+   return check_expected_revs(argv, argc);
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 442397b..c3e43248 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -237,22 +237,6 @@ bisect_write() {
test -n "$nolog" || echo "git bisect $state $rev" 
>>"$GIT_DIR/BISECT_LOG"
 }
 
-is_expected_rev() {
-   test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
-   test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")
-}
-
-check_expected_revs() {
-   for _rev in "$@"; do
-   if ! is_expected_rev "$_rev"
-   then
-   rm -f "$GIT_DIR/BISECT_ANCESTORS_OK"
-   rm -f "$GIT_DIR/BISECT_EXPECTED_REV"
-   return
-   fi
-   done
-}
-
 bisect_skip() {
all=''
for arg in "$@"
@@ -280,7 +264,7 @@ bisect_state() {
rev=$(git rev-parse --verify "$bisected_head") ||
die "$(eval_gettext "Bad rev input: \$bisected_head")"
bisect_write "$state" "$rev"
-   check_expected_revs "$rev" ;;
+   git bisect--helper --check-expected-revs "$rev" ;;
2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
shift
hash_list=''
@@ -294,7 +278,7 @@ bisect_state() {
do
bisect_write "$state" "$rev"
done
-   check_expected_revs $hash_list ;;
+   git bisect--helper --check-expected-revs $hash_list ;;
*,"$TERM_BAD")
die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one 
argument.")" ;;
*)

--
https://github.com/git/git/pull/281
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More 

[PATCH v13 09/13] bisect--helper: `bisect_write` shell function in C

[Pranit Bauva]

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

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

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

This patch also introduces new methods namely bisect_state_init() and
bisect_terms_release() for easy memory management for the struct
bisect_terms.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 97 
 git-bisect.sh| 25 ++---
 2 files changed, 94 insertions(+), 28 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 711be75..5364960 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -19,9 +19,27 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --write-terms  "),
N_("git bisect--helper --bisect-clean-state"),
N_("git bisect--helper --bisect-reset []"),
+   N_("git bisect--helper --bisect-write
 []"),
NULL
 };
 
+struct bisect_terms {
+   struct strbuf term_good;
+   struct strbuf term_bad;
+};
+
+static void bisect_terms_init(struct bisect_terms *terms)
+{
+   strbuf_init(>term_good, 0);
+   strbuf_init(>term_bad, 0);
+}
+
+static void bisect_terms_release(struct bisect_terms *terms)
+{
+   strbuf_release(>term_good);
+   strbuf_release(>term_bad);
+}
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -149,6 +167,52 @@ static int check_expected_revs(const char **revs, int 
rev_nr)
return 0;
 }
 
+static int bisect_write(const char *state, const char *rev,
+   const struct bisect_terms *terms, int nolog)
+{
+   struct strbuf tag = STRBUF_INIT;
+   struct strbuf commit_name = STRBUF_INIT;
+   struct object_id oid;
+   struct commit *commit;
+   struct pretty_print_context pp = {0};
+   FILE *fp;
+
+   if (!strcmp(state, terms->term_bad.buf))
+   strbuf_addf(, "refs/bisect/%s", state);
+   else if (one_of(state, terms->term_good.buf, "skip", NULL))
+   strbuf_addf(, "refs/bisect/%s-%s", state, rev);
+   else
+   return error(_("Bad bisect_write argument: %s"), state);
+
+   if (get_oid(rev, )) {
+   strbuf_release();
+   return error(_("couldn't get the oid of the rev '%s'"), rev);
+   }
+
+   if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
+  UPDATE_REFS_MSG_ON_ERR)) {
+   strbuf_release();
+   return -1;
+   }
+   strbuf_release();
+
+   fp = fopen(git_path_bisect_log(), "a");
+   if (!fp)
+   return error_errno(_("couldn't open the file '%s'"), 
git_path_bisect_log());
+
+   commit = lookup_commit_reference(oid.hash);
+   format_commit_message(commit, "%s", _name, );
+   fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
+   commit_name.buf);
+   strbuf_release(_name);
+
+   if (!nolog)
+   fprintf(fp, "git bisect %s %s\n", state, rev);
+
+   fclose(fp);
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -156,9 +220,10 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
WRITE_TERMS,
BISECT_CLEAN_STATE,
BISECT_RESET,
-   CHECK_EXPECTED_REVS
+   CHECK_EXPECTED_REVS,
+   BISECT_WRITE
} cmdmode = 0;
-   int no_checkout = 0;
+   int no_checkout = 0, res = 0;
struct option options[] = {
OPT_CMDMODE(0, "next-all", ,
 N_("perform 'git bisect next'"), NEXT_ALL),
@@ -170,10 +235,14 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("reset the bisection state"), BISECT_RESET),
OPT_CMDMODE(0, "check-expected-revs", ,
 N_("check for expected revs"), CHECK_EXPECTED_REVS),
+   OPT_CMDMODE(0, "bisect-write", ,
+N_("write out the bisection state in BISECT_LOG"), 
BISECT_WRITE),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),

[PATCH v13 12/13] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Pranit Bauva]

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

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

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

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index ada3220d..e1cf956 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -23,6 +23,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-write
 []"),
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
N_("git bisect--helper --bisect-next-check []  
term_bad, fp) == EOF ||
+ strbuf_getline(>term_good, fp) == EOF;
+
+   fclose(fp);
+   return res ? -1 : 0;
+}
+
+static int bisect_terms(struct bisect_terms *terms, const char **argv, int 
argc)
+{
+   int i;
+
+   if (get_terms(terms)) {
+   fprintf(stderr, N_("no terms defined\n"));
+   return -1;
+   }
+   if (argc == 0) {
+   printf(N_("Your current terms are %s for the old state\nand "
+  "%s for the new state.\n"), terms->term_good.buf,
+  terms->term_bad.buf);
+   return 0;
+   }
+
+   for (i = 0; i < argc; i++) {
+   if (!strcmp(argv[i], "--term-good"))
+   printf(N_("%s\n"), terms->term_good.buf);
+   else if (!strcmp(argv[i], "--term-bad"))
+   printf(N_("%s\n"), terms->term_bad.buf);
+   else
+   printf(N_("invalid argument %s for 'git bisect "
+ "terms'.\nSupported options are: "
+ "--term-good|--term-old and "
+ "--term-bad|--term-new."), argv[i]);
+   }
+
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -354,7 +401,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
CHECK_EXPECTED_REVS,
BISECT_WRITE,
CHECK_AND_SET_TERMS,
-   BISECT_NEXT_CHECK
+   BISECT_NEXT_CHECK,
+   BISECT_TERMS
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -374,6 +422,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("check and set terms in a bisection state"), 
CHECK_AND_SET_TERMS),
OPT_CMDMODE(0, "bisect-next-check", ,
 N_("check whether bad or good terms exist"), 
BISECT_NEXT_CHECK),
+   OPT_CMDMODE(0, "bisect-terms", ,
+N_("print out the bisect terms"), BISECT_TERMS),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -382,7 +432,7 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
bisect_terms_init();
 
argc = parse_options(argc, argv, prefix, options,
-git_bisect_helper_usage, 0);
+git_bisect_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
 
if (!cmdmode)
usage_with_options(git_bisect_helper_usage, options);
@@ -431,6 +481,11 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
strbuf_addstr(_bad, argv[1]);
res = bisect_next_check(, argc == 3 ? argv[2] : NULL);
break;
+   case BISECT_TERMS:
+   if (argc > 1)
+   die(_("--bisect-terms requires 0 or 1 argument"));
+   res = bisect_terms(, argv, argc);
+   break;
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh 

[PATCH v13 11/13] bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Pranit Bauva]

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

Also reimplement `bisect_voc` shell function in C and call it from
`bisect_next_check` implementation in C.

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

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

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 450426c..ada3220d 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -6,6 +6,7 @@
 #include "dir.h"
 #include "argv-array.h"
 #include "run-command.h"
+#include "prompt.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -21,6 +22,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-reset []"),
N_("git bisect--helper --bisect-write
 []"),
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
+   N_("git bisect--helper --bisect-next-check []  
term_bad.buf);
+   char *good_glob = xstrfmt("%s-*", terms->term_good.buf);
+   char *bad_syn, *good_syn;
+
+   if (ref_exists(bad_ref))
+   missing_bad = 0;
+   free(bad_ref);
+
+   for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
+(void *) _good);
+   free(good_glob);
+
+   if (!missing_good && !missing_bad)
+   return 0;
+
+   if (!current_term)
+   return -1;
+
+   if (missing_good && !missing_bad && current_term &&
+   !strcmp(current_term, terms->term_good.buf)) {
+   char *yesno;
+   /*
+* have bad (or new) but not good (or old). We could bisect
+* although this is less optimum.
+*/
+   fprintf(stderr, N_("Warning: bisecting only with a %s 
commit\n"),
+   terms->term_bad.buf);
+   if (!isatty(0))
+   return 0;
+   /*
+* TRANSLATORS: Make sure to include [Y] and [n] in your
+* translation. The program will only accept English input
+* at this point.
+*/
+   yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
+   if (starts_with(yesno, "N") || starts_with(yesno, "n"))
+   return -1;
+
+   return 0;
+   }
+   bad_syn = xstrdup(bisect_voc("bad"));
+   good_syn = xstrdup(bisect_voc("good"));
+   if (!is_empty_or_missing_file(git_path_bisect_start())) {
+   error(_("You need to give me at least one %s and "
+   "%s revision. You can use \"git bisect %s\" "
+   "and \"git bisect %s\" for that. \n"),
+   bad_syn, good_syn, bad_syn, good_syn);
+   free(bad_syn);
+   free(good_syn);
+   return -1;
+   }
+   else {
+   error(_("You need to start by \"git bisect start\". You "
+   "then need to give me at least one %s and %s "
+   "revision. You can use \"git bisect %s\" and "
+   "\"git bisect %s\" for that.\n"),
+   good_syn, bad_syn, bad_syn, good_syn);
+   free(bad_syn);
+   free(good_syn);
+   return -1;
+   }
+   free(bad_syn);
+   free(good_syn);
+
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -262,7 +353,8 @@ int cmd_bisect__helper(int argc, 

Re: [PATCH v8 4/8] submodule: allow add_submodule_odb to work even if path is not checked out

[Jacob Keller]

On Fri, Aug 19, 2016 at 1:06 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> diff --git a/path.c b/path.c
>> index 17551c483476..0cb30123e988 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -482,6 +482,11 @@ 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)) {
>> + strbuf_reset(buf);
>> + strbuf_git_path(buf, "%s/%s", "modules", path);
>> + }
>> +
>>   strbuf_addch(buf, '/');
>>   strbuf_addbuf(_submodule_dir, buf);
>
> So, given submodule at $path, so far we have tried $path/.git and
> would have been happy if it was already a directory (i.e. an
> embedded repository in place), would have been happy if it was
> pointing at elsewhere (presumably at .git/modules/$name) that is a
> directory, and this is a fallback that covers the case where $path
> in the working tree of the superproject is missing.
>
> I _think_ "path" needs to be mapped to the "name" of the submodule
> that should be at the "path".  Other than that, this hunk looks
> correct to me.

How do we do that? I tried to find a way to do that, couldn't, and
decided it was probably already normalized. It seems to be, based on
my tests, where I run git-log from within a subdirectory that has the
submodule. I think we already have the complete path. Or is the name
*not* equivalent to the path?

>
>> diff --git a/submodule.c b/submodule.c
>> index 1b5cdfb7e784..e1a51b7506ff 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -348,7 +348,7 @@ void show_submodule_summary(FILE *f, const char *path,
>>   if (is_null_sha1(two))
>>   message = "(submodule deleted)";
>>   else if (add_submodule_odb(path))
>> - message = "(not checked out)";
>> + message = "(not initialized)";
>>   else if (is_null_sha1(one))
>>   message = "(new submodule)";
>>   else if (!(left = lookup_commit_reference(one)) ||
>
> This is a change unrelated to the fix you made above, and should be
> done in its own separate patch, I would think.
>

It is the same change. The first hunk makes it so that "not checked
out" is no longer what occurs. We change the behavior of
add_submodule_odb into "work even if there is no check out, but only
fail when the submodule isn't initialized at all, either in path/.git
or ./git/modules/path/" so we need to change the message because we've
changed behavior.

>> diff --git a/t/t4059-diff-submodule-not-initialized.sh 
>> b/t/t4059-diff-submodule-not-initialized.sh
>> new file mode 100755
>> index ..cc787454033a
>> --- /dev/null
>> +++ b/t/t4059-diff-submodule-not-initialized.sh
>> @@ -0,0 +1,105 @@
>> +#!/bin/sh
>> +#
>> +# Copyright (c) 2016 Jacob Keller, based on t4041 by Jens Lehmann
>> +#
>> +
>> +test_description='Test for submodule diff on non-checked out submodule
>> +
>> +This test tries to verify that add_submodule_odb works when the submodule 
>> was
>> +initialized previously but the checkout has since been removed.
>> +'
>> +
>> +TEST_NO_CREATE_REPO=1
>> +. ./test-lib.sh
>> +
>> +# Tested non-UTF-8 encoding
>> +test_encoding="ISO8859-1"
>> +
>> +# String "added" in German (translated with Google Translate), encoded in 
>> UTF-8,
>> +# used in sample commit log messages in add_file() function below.
>> +added=$(printf "hinzugef\303\274gt")
>
> Have an empty line here?
>

Makes sense I suppose.

>> +add_file () {
>> + (
>> + cd "$1" &&
>> + shift &&
>> + for name
>> + do
>> + echo "$name" >"$name" &&
>> + git add "$name" &&
>> + test_tick &&
>> + # "git commit -m" would break MinGW, as Windows refuse 
>> to pass
>> + # $test_encoding encoded parameter to git.
>> + echo "Add $name ($added $name)" | iconv -f utf-8 -t 
>> $test_encoding |
>> + git -c "i18n.commitEncoding=$test_encoding" commit -F -
>> + done >/dev/null &&
>> + git rev-parse --short --verify HEAD
>> + )
>> +}
>
> Have an empty line here?
>

There was no empty line in the place I copied from.

>> +commit_file () {
>> + test_tick &&
>> + git commit "$@" -m "Commit $*" >/dev/null
>> +}
>
> Surround these ...
>
>> +test_create_repo sm1 &&
>> +test_create_repo sm2 &&
>> +add_file sm1 foo >/dev/null &&
>> +add_file sm2 foo1 foo2 >/dev/null &&
>> +smhead1=$(cd sm2; git rev-parse --short --verify HEAD) &&
>
> ... inside its own "test_expect_success setup"?
>
> None of the ">/dev/null" we see in the above helper functions (and
> use of these helper functions) are necessary or desired, I would
> think.

If we put them in test_expect_success setup they aren't. They were
because add_file returns the sha1 of the rev-parse on HEAD after
adding.

>
> The 

Re: [PATCH v8 7/8] cache: add empty_tree_oid object

[Junio C Hamano]

Jacob Keller  writes:

> Is there a reason for that? I've found that .field = value is safer
> because it ensures that you don't end up initializing the wrong
> values? Or is it a compatibility thing?

Yes.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v13 01/13] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

`--next-all` is meant to be used as a subcommand to support multiple
"operation mode" though the current implementation does not contain any
other subcommand along side with `--next-all` but further commits will
include some more subcommands.

Helped-by: Johannes Schindelin 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..8111c91 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -10,11 +10,11 @@ static const char * const git_bisect_helper_usage[] = {
 
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-   int next_all = 0;
+   enum { NEXT_ALL = 1 } cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
-   OPT_BOOL(0, "next-all", _all,
-N_("perform 'git bisect next'")),
+   OPT_CMDMODE(0, "next-all", ,
+N_("perform 'git bisect next'"), NEXT_ALL),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
argc = parse_options(argc, argv, prefix, options,
 git_bisect_helper_usage, 0);
 
-   if (!next_all)
+   if (!cmdmode)
usage_with_options(git_bisect_helper_usage, options);
 
-   /* next-all */
-   return bisect_next_all(prefix, no_checkout);
+   switch (cmdmode) {
+   case NEXT_ALL:
+   return bisect_next_all(prefix, no_checkout);
+   default:
+   die("BUG: unknown subcommand '%d'", cmdmode);
+   }
+   return 0;
 }

--
https://github.com/git/git/pull/281
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v13 07/13] bisect--helper: `bisect_reset` shell function in C

[Pranit Bauva]

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

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

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

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 48 +++-
 git-bisect.sh| 28 ++--
 2 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index e50934c..9aba094 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -3,17 +3,22 @@
 #include "parse-options.h"
 #include "bisect.h"
 #include "refs.h"
+#include "dir.h"
+#include "argv-array.h"
+#include "run-command.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
+static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms  "),
N_("git bisect--helper --bisect-clean-state"),
+   N_("git bisect--helper --bisect-reset []"),
NULL
 };
 
@@ -84,12 +89,47 @@ static int write_terms(const char *bad, const char *good)
return (res < 0) ? -1 : 0;
 }
 
+static int bisect_reset(const char *commit)
+{
+   struct strbuf branch = STRBUF_INIT;
+
+   if (!commit) {
+   if (strbuf_read_file(, git_path_bisect_start(), 0) < 1) {
+   printf("We are not bisecting.\n");
+   return 0;
+   }
+   strbuf_rtrim();
+   } else {
+   struct object_id oid;
+   if (get_oid(commit, ))
+   return error(_("'%s' is not a valid commit"), commit);
+   strbuf_addstr(, commit);
+   }
+
+   if (!file_exists(git_path_bisect_head())) {
+   struct argv_array argv = ARGV_ARRAY_INIT;
+   argv_array_pushl(, "checkout", branch.buf, "--", NULL);
+   if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
+   error(_("Could not check out original HEAD '%s'. Try "
+   "'git bisect reset '."), branch.buf);
+   strbuf_release();
+   argv_array_clear();
+   return -1;
+   }
+   argv_array_clear();
+   }
+
+   strbuf_release();
+   return bisect_clean_state();
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
NEXT_ALL = 1,
WRITE_TERMS,
-   BISECT_CLEAN_STATE
+   BISECT_CLEAN_STATE,
+   BISECT_RESET
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -99,6 +139,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
OPT_CMDMODE(0, "bisect-clean-state", ,
 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
+   OPT_CMDMODE(0, "bisect-reset", ,
+N_("reset the bisection state"), BISECT_RESET),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -121,6 +163,10 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
if (argc != 0)
die(_("--bisect-clean-state requires no arguments"));
return bisect_clean_state();
+   case BISECT_RESET:
+   if (argc > 1)
+   die(_("--bisect-reset requires either zero or one 
arguments"));
+   return bisect_reset(argc ? argv[0] : NULL);
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index f1202df..442397b 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -409,35 +409,11 @@ bisect_visualize() {
eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
 }
 
-bisect_reset() {
-   test -s "$GIT_DIR/BISECT_START" 

Re: [PATCH] diff-highlight: add some tests.

[Junio C Hamano]

Brian Henderson  writes:

> Junio, how does this look?
> ...
> +# dh_test is a test helper function which takes 1) some file data, 2) some
> +# change of the file data, creates a diff and commit of the changes and 
> passes
> +# that through diff-highlight.
> +# The optional 3rd parameter is the expected output of diff-highlight minus 
> the
> +# diff/commit header. This parameter is given directly to printf as the 
> format
> +# string (in order to properly handle ascii escape codes; CW, CR), so any '%'
> +# need to be doubled to protect it.
> +# Don't include a 3rd parameter if diff-highlight is supposed to leave the
> +# input unmodified.
> +# For convienence, the 3rd parameter can begin with a newline which will be
> +# stripped.

You seem to be stripping any and all empty lines with "perl -pe"; I
am not sure if that is sensible.

I really do not see the point of being able to spell

"
aaa
bbb
"

when you can perfectly well read

"aaa
bbb"

or even "aaa\nbbb\n" for that matter.  I personally do not think the
difference is worth the cost of an extra invocation of Perl, but we
already saw how stubborn you are, so there is no point spending my
time on trying to convince you further.  Assuming that it is so
precious that the input can start with an extra blank line, what you
wrote is a sensible implementation.

> +dh_test () {
> + a="$1" b="$2" &&
> +
> + {
> + printf "$a" >file &&
> + git add file &&
> + git commit -m "Add a file" &&
> +
> + printf "$b" >file &&
> + git diff file >diff.raw &&
> + git commit -am "Update a file" &&
> + git show >commit.raw
> + } >/dev/null &&
> +
> + if test $# -ge 3

I think

if test $# = 3

is much better; you would want to catch bugs in a caller that gave
you fourth parameter by mistake, instead of silently ignoring it.

> +test_strip_patch_header () {
> + sed -e '1,/^@@/d' "$@"
> +}

While I think it is a great idea to hide the sed command behind a
helper function for readability, I am not sure "strip patch header"
is a great name that describes what it does.  A natural expectation
for an operation with that name done to this input:

diff --git a/file b/file
index fff..fff 100644
--- a/file
+++ b/file
@@ -l,k +m,n @@ comments
 common
-old
+new
 common
@@ -l,k +m,n @@ more comments
 common
+added more
+added even more

would be to remove the first four lines, not five.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v13 13/13] bisect--helper: `bisect_start` shell function partially in C

[Pranit Bauva]

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

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

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

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 252 +--
 git-bisect.sh| 133 +
 2 files changed, 246 insertions(+), 139 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index e1cf956..ff1dfc7 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -7,6 +7,7 @@
 #include "argv-array.h"
 #include "run-command.h"
 #include "prompt.h"
+#include "quote.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -14,6 +15,8 @@ static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, 
"BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
+static GIT_PATH_FUNC(git_path_head_name, "head-name")
+static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
@@ -24,6 +27,8 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
N_("git bisect--helper --bisect-next-check []  
term_bad.buf);
if (!isatty(0))
return 0;
@@ -366,11 +371,11 @@ static int bisect_terms(struct bisect_terms *terms, const 
char **argv, int argc)
int i;
 
if (get_terms(terms)) {
-   fprintf(stderr, N_("no terms defined\n"));
+   fprintf(stderr, _("no terms defined\n"));
return -1;
}
if (argc == 0) {
-   printf(N_("Your current terms are %s for the old state\nand "
+   printf(_("Your current terms are %s for the old state\nand "
   "%s for the new state.\n"), terms->term_good.buf,
   terms->term_bad.buf);
return 0;
@@ -378,11 +383,11 @@ static int bisect_terms(struct bisect_terms *terms, const 
char **argv, int argc)
 
for (i = 0; i < argc; i++) {
if (!strcmp(argv[i], "--term-good"))
-   printf(N_("%s\n"), terms->term_good.buf);
+   printf(_("%s\n"), terms->term_good.buf);
else if (!strcmp(argv[i], "--term-bad"))
-   printf(N_("%s\n"), terms->term_bad.buf);
+   printf(_("%s\n"), terms->term_bad.buf);
else
-   printf(N_("invalid argument %s for 'git bisect "
+   printf(_("invalid argument %s for 'git bisect "
  "terms'.\nSupported options are: "
  "--term-good|--term-old and "
  "--term-bad|--term-new."), argv[i]);
@@ -391,6 +396,228 @@ static int bisect_terms(struct bisect_terms *terms, const 
char **argv, int argc)
return 0;
 }
 
+static int bisect_start(struct bisect_terms *terms, int no_checkout,
+   const char **argv, int argc)
+{
+   int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
+   int flags, pathspec_pos;
+   struct string_list revs = STRING_LIST_INIT_DUP;
+   struct string_list states = STRING_LIST_INIT_DUP;
+   struct strbuf start_head = STRBUF_INIT;
+   struct strbuf bisect_names = STRBUF_INIT;
+   struct strbuf orig_args = STRBUF_INIT;
+   const char *head;
+   

[PATCH v13 02/13] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

Reimplement the `check_term_format` shell function in C and add
a `--check-term-format` subcommand to `git bisect--helper` to call it
from git-bisect.sh

Using `--check-term-format` subcommand is a temporary measure to port
shell function to C so as to use the existing test suite. As more
functions are ported, this subcommand will be retired and its
implementation will be called by some other method/subcommand. For
eg. In conversion of write_terms() of git-bisect.sh, the subcommand will
be removed and instead check_term_format() will be called in its C
implementation while a new subcommand will be introduced for write_terms().

Helped-by: Johannes Schindelein 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 60 +++-
 git-bisect.sh| 31 ++---
 2 files changed, 61 insertions(+), 30 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8111c91..a47f1f2 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,19 +2,73 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "bisect.h"
+#include "refs.h"
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
+   N_("git bisect--helper --check-term-format  "),
NULL
 };
 
+/*
+ * Check whether the string `term` belongs to the set of strings
+ * included in the variable arguments.
+ */
+LAST_ARG_MUST_BE_NULL
+static int one_of(const char *term, ...)
+{
+   int res = 0;
+   va_list matches;
+   const char *match;
+
+   va_start(matches, term);
+   while (!res && (match = va_arg(matches, const char *)))
+   res = !strcmp(term, match);
+   va_end(matches);
+
+   return res;
+}
+
+static int check_term_format(const char *term, const char *orig_term)
+{
+   int res;
+   char *new_term = xstrfmt("refs/bisect/%s", term);
+
+   res = check_refname_format(new_term, 0);
+   free(new_term);
+
+   if (res)
+   return error(_("'%s' is not a valid term"), term);
+
+   if (one_of(term, "help", "start", "skip", "next", "reset",
+   "visualize", "replay", "log", "run", NULL))
+   return error(_("can't use the builtin command '%s' as a term"), 
term);
+
+   /*
+* In theory, nothing prevents swapping completely good and bad,
+* but this situation could be confusing and hasn't been tested
+* enough. Forbid it for now.
+*/
+
+   if ((strcmp(orig_term, "bad") && one_of(term, "bad", "new", NULL)) ||
+(strcmp(orig_term, "good") && one_of(term, "good", "old", 
NULL)))
+   return error(_("can't change the meaning of the term '%s'"), 
term);
+
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-   enum { NEXT_ALL = 1 } cmdmode = 0;
+   enum {
+   NEXT_ALL = 1,
+   CHECK_TERM_FMT
+   } cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
OPT_CMDMODE(0, "next-all", ,
 N_("perform 'git bisect next'"), NEXT_ALL),
+   OPT_CMDMODE(0, "check-term-format", ,
+N_("check format of the term"), CHECK_TERM_FMT),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -29,6 +83,10 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
switch (cmdmode) {
case NEXT_ALL:
return bisect_next_all(prefix, no_checkout);
+   case CHECK_TERM_FMT:
+   if (argc != 2)
+   die(_("--check-term-format requires two arguments"));
+   return check_term_format(argv[0], argv[1]);
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index ae3cb01..a727c59 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,38 +564,11 @@ write_terms () {
then
die "$(gettext "please use two different terms")"
fi
-   check_term_format "$TERM_BAD" bad
-   check_term_format "$TERM_GOOD" good
+   git bisect--helper --check-term-format "$TERM_BAD" bad || exit
+   git bisect--helper --check-term-format "$TERM_GOOD" good || exit
printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 
-check_term_format () {
-   term=$1
-   git check-ref-format refs/bisect/"$term" ||
-   die "$(eval_gettext "'\$term' is not a valid term")"
-   case "$term" in
-   help|start|terms|skip|next|reset|visualize|replay|log|run)
-   die 

Re: [PATCH v8 7/8] cache: add empty_tree_oid object

[Jacob Keller]

On Fri, Aug 19, 2016 at 1:31 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> From: Jacob Keller 
>>
>> Add an empty_tree_oid object which can be used in place of
>> EMPTY_TREE_SHA1_BIN_LITERAL for code which is being converted to struct
>> object_id.
>
> How widely do you envision the users of this symbol would be spread
> across the entire codebase?  I am debating myself if we need a
> singleton in-core copy like this (if we end up referring to it from
> everywhere), or we only need one level higher abstraction,
> e.g. "is_empty_tree_oid()" helper (in which case I do not think we
> even need a singleton; just imitate how is_empty_blob_sha1() is
> implemented).

If I do this, I'd also add an "is_empty_tree_sha1()" as well?

>
> Even if we need such a singleton, I think we avoid ".field = value"
> struct initializations in our code.
>

Is there a reason for that? I've found that .field = value is safer
because it ensures that you don't end up initializing the wrong
values? Or is it a compatibility thing?

Thanks,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 6/8] submodule: refactor show_submodule_summary with helper function

[Jacob Keller]

On Fri, Aug 19, 2016 at 1:24 PM, Junio C Hamano  wrote:
> And probably merge_bases also leaks here.
>
> It is not cheap to compute merge bases, but show_submodule_summary()
> makes two calls to get_merge_bases(), one in show_submodule_header()
> and then another inside prepare_submodule_summary() to compute
> exactly the same set of merge bases.  We somehow need to reduce it
> to just one.
>

I can make show_submodule_headers take another parameter which we
pass, and then pass that into prepare_submodule_summary...?

Thanks,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 7/8] cache: add empty_tree_oid object

[Junio C Hamano]

Jacob Keller  writes:

> From: Jacob Keller 
>
> Add an empty_tree_oid object which can be used in place of
> EMPTY_TREE_SHA1_BIN_LITERAL for code which is being converted to struct
> object_id.

How widely do you envision the users of this symbol would be spread
across the entire codebase?  I am debating myself if we need a
singleton in-core copy like this (if we end up referring to it from
everywhere), or we only need one level higher abstraction,
e.g. "is_empty_tree_oid()" helper (in which case I do not think we
even need a singleton; just imitate how is_empty_blob_sha1() is
implemented).

Even if we need such a singleton, I think we avoid ".field = value"
struct initializations in our code.

>
> Signed-off-by: Jacob Keller 
> ---
>  cache.h | 2 ++
>  sha1_file.c | 3 +++
>  2 files changed, 5 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index f30a4417efdf..da9f0be67d7b 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -964,6 +964,8 @@ static inline void oidclr(struct object_id *oid)
>  #define EMPTY_BLOB_SHA1_BIN \
>   ((const unsigned char *) EMPTY_BLOB_SHA1_BIN_LITERAL)
>  
> +extern const struct object_id empty_tree_oid;
> +
>  static inline int is_empty_blob_sha1(const unsigned char *sha1)
>  {
>   return !hashcmp(sha1, EMPTY_BLOB_SHA1_BIN);
> diff --git a/sha1_file.c b/sha1_file.c
> index 1e23fc186a02..10883d56a600 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -38,6 +38,9 @@ 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 = {
> + .hash = EMPTY_TREE_SHA1_BIN_LITERAL
> +};
>  
>  /*
>   * This is meant to hold a *small* number of objects that you would
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 6/8] submodule: refactor show_submodule_summary with helper function

[Junio C Hamano]

Jacob Keller  writes:

> @@ -290,12 +289,6 @@ static int prepare_submodule_summary(struct rev_info 
> *rev, const char *path,
>   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,

Not a new issue with this patch, but I wonder if this commit_list is
leaking here.

> + /*
> +  * Warn about missing commits in the submodule project, but only if
> +  * they aren't null.
> +  */
> + if ((!is_null_oid(one) && !*left) ||
> +  (!is_null_oid(two) && !*right))
> + message = "(commits not present)";
> +
> + 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;
> + }

And probably merge_bases also leaks here.

It is not cheap to compute merge bases, but show_submodule_summary()
makes two calls to get_merge_bases(), one in show_submodule_header()
and then another inside prepare_submodule_summary() to compute
exactly the same set of merge bases.  We somehow need to reduce it
to just one.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] diff-highlight: add some tests.

[Brian Henderson]

Junio, how does this look?

Signed-off-by: Brian Henderson 
---
 contrib/diff-highlight/Makefile  |   5 +
 contrib/diff-highlight/t/Makefile|  22 
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 122 +++
 3 files changed, 149 insertions(+)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh

diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
new file mode 100644
index 000..9018724
--- /dev/null
+++ b/contrib/diff-highlight/Makefile
@@ -0,0 +1,5 @@
+# nothing to build
+all:
+
+test:
+   $(MAKE) -C t
diff --git a/contrib/diff-highlight/t/Makefile 
b/contrib/diff-highlight/t/Makefile
new file mode 100644
index 000..5ff5275
--- /dev/null
+++ b/contrib/diff-highlight/t/Makefile
@@ -0,0 +1,22 @@
+-include ../../../config.mak.autogen
+-include ../../../config.mak
+
+# copied from ../../t/Makefile
+SHELL_PATH ?= $(SHELL)
+SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
+
+all: test
+test: $(T)
+
+.PHONY: help clean all test $(T)
+
+help:
+   @echo 'Run "$(MAKE) test" to launch test scripts'
+   @echo 'Run "$(MAKE) clean" to remove trash folders'
+
+$(T):
+   @echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+
+clean:
+   $(RM) -r 'trash directory'.*
diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh 
b/contrib/diff-highlight/t/t9400-diff-highlight.sh
new file mode 100755
index 000..3c04116
--- /dev/null
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -0,0 +1,122 @@
+#!/bin/sh
+
+test_description='Test diff-highlight'
+
+CURR_DIR=$(pwd)
+TEST_OUTPUT_DIRECTORY=$(pwd)
+TEST_DIRECTORY="$CURR_DIR"/../../../t
+DIFF_HIGHLIGHT="$CURR_DIR"/../diff-highlight
+
+CW="\033[7m"   # white
+CR="\033[27m" # reset
+
+. "$TEST_DIRECTORY"/test-lib.sh
+
+if ! test_have_prereq PERL
+then
+   skip_all='skipping diff-highlight tests; perl not available'
+   test_done
+fi
+
+# dh_test is a test helper function which takes 1) some file data, 2) some
+# change of the file data, creates a diff and commit of the changes and passes
+# that through diff-highlight.
+# The optional 3rd parameter is the expected output of diff-highlight minus the
+# diff/commit header. This parameter is given directly to printf as the format
+# string (in order to properly handle ascii escape codes; CW, CR), so any '%'
+# need to be doubled to protect it.
+# Don't include a 3rd parameter if diff-highlight is supposed to leave the
+# input unmodified.
+# For convienence, the 3rd parameter can begin with a newline which will be
+# stripped.
+dh_test () {
+   a="$1" b="$2" &&
+
+   {
+   printf "$a" >file &&
+   git add file &&
+   git commit -m "Add a file" &&
+
+   printf "$b" >file &&
+   git diff file >diff.raw &&
+   git commit -am "Update a file" &&
+   git show >commit.raw
+   } >/dev/null &&
+
+   if test $# -ge 3
+   then
+   # strip optional beginning newline
+   printf "$3" | perl -pe 's/^\n//'
+   else
+   test_strip_patch_header diff.raw
+   fi >patch.exp &&
+
+   "$DIFF_HIGHLIGHT" diff.act &&
+   "$DIFF_HIGHLIGHT" commit.act &&
+   test_cmp patch.exp diff.act &&
+   test_cmp patch.exp commit.act
+}
+
+test_strip_patch_header () {
+   sed -e '1,/^@@/d' "$@"
+}
+
+test_expect_success 'diff-highlight highlights the beginning of a line' '
+   dh_test \
+   "aaa\nbbb\nccc\n" \
+   "aaa\n0bb\nccc\n" \
+"
+ aaa
+-${CW}b${CR}bb
++${CW}0${CR}bb
+ ccc
+"
+'
+
+test_expect_success 'diff-highlight highlights the end of a line' '
+   dh_test \
+   "aaa\nbbb\nccc\n" \
+   "aaa\nbb0\nccc\n" \
+"
+ aaa
+-bb${CW}b${CR}
++bb${CW}0${CR}
+ ccc
+"
+'
+
+test_expect_success 'diff-highlight highlights the middle of a line' '
+   dh_test \
+   "aaa\nbbb\nccc\n" \
+   "aaa\nb0b\nccc\n" \
+"
+ aaa
+-b${CW}b${CR}b
++b${CW}0${CR}b
+ ccc
+"
+'
+
+test_expect_success 'diff-highlight does not highlight whole line' '
+   dh_test \
+   "aaa\nbbb\nccc\n" \
+   "aaa\n000\nccc\n"
+'
+
+test_expect_failure 'diff-highlight highlights mismatched hunk size' '
+   dh_test \
+   "aaa\nbbb\n" \
+   "aaa\nb0b\nccc\n" \
+"
+ aaa
+-b${CW}b${CR}b
++b${CW}0${CR}b
++ccc
+"
+'
+
+# TODO add multi-byte test
+
+test_done
+
+# vim: set noet
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 5/8] submodule: convert show_submodule_summary to use struct object_id *

[Junio C Hamano]

Jacob Keller  writes:

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

Sounds good.  Thanks.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 4/8] submodule: allow add_submodule_odb to work even if path is not checked out

[Junio C Hamano]

Jacob Keller  writes:

> diff --git a/path.c b/path.c
> index 17551c483476..0cb30123e988 100644
> --- a/path.c
> +++ b/path.c
> @@ -482,6 +482,11 @@ 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)) {
> + strbuf_reset(buf);
> + strbuf_git_path(buf, "%s/%s", "modules", path);
> + }
> +
>   strbuf_addch(buf, '/');
>   strbuf_addbuf(_submodule_dir, buf);

So, given submodule at $path, so far we have tried $path/.git and
would have been happy if it was already a directory (i.e. an
embedded repository in place), would have been happy if it was
pointing at elsewhere (presumably at .git/modules/$name) that is a
directory, and this is a fallback that covers the case where $path
in the working tree of the superproject is missing.

I _think_ "path" needs to be mapped to the "name" of the submodule
that should be at the "path".  Other than that, this hunk looks
correct to me.

> diff --git a/submodule.c b/submodule.c
> index 1b5cdfb7e784..e1a51b7506ff 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -348,7 +348,7 @@ void show_submodule_summary(FILE *f, const char *path,
>   if (is_null_sha1(two))
>   message = "(submodule deleted)";
>   else if (add_submodule_odb(path))
> - message = "(not checked out)";
> + message = "(not initialized)";
>   else if (is_null_sha1(one))
>   message = "(new submodule)";
>   else if (!(left = lookup_commit_reference(one)) ||

This is a change unrelated to the fix you made above, and should be
done in its own separate patch, I would think.

> diff --git a/t/t4059-diff-submodule-not-initialized.sh 
> b/t/t4059-diff-submodule-not-initialized.sh
> new file mode 100755
> index ..cc787454033a
> --- /dev/null
> +++ b/t/t4059-diff-submodule-not-initialized.sh
> @@ -0,0 +1,105 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2016 Jacob Keller, based on t4041 by Jens Lehmann
> +#
> +
> +test_description='Test for submodule diff on non-checked out submodule
> +
> +This test tries to verify that add_submodule_odb works when the submodule was
> +initialized previously but the checkout has since been removed.
> +'
> +
> +TEST_NO_CREATE_REPO=1
> +. ./test-lib.sh
> +
> +# Tested non-UTF-8 encoding
> +test_encoding="ISO8859-1"
> +
> +# String "added" in German (translated with Google Translate), encoded in 
> UTF-8,
> +# used in sample commit log messages in add_file() function below.
> +added=$(printf "hinzugef\303\274gt")

Have an empty line here?

> +add_file () {
> + (
> + cd "$1" &&
> + shift &&
> + for name
> + do
> + echo "$name" >"$name" &&
> + git add "$name" &&
> + test_tick &&
> + # "git commit -m" would break MinGW, as Windows refuse 
> to pass
> + # $test_encoding encoded parameter to git.
> + echo "Add $name ($added $name)" | iconv -f utf-8 -t 
> $test_encoding |
> + git -c "i18n.commitEncoding=$test_encoding" commit -F -
> + done >/dev/null &&
> + git rev-parse --short --verify HEAD
> + )
> +}

Have an empty line here?

> +commit_file () {
> + test_tick &&
> + git commit "$@" -m "Commit $*" >/dev/null
> +}

Surround these ...

> +test_create_repo sm1 &&
> +test_create_repo sm2 &&
> +add_file sm1 foo >/dev/null &&
> +add_file sm2 foo1 foo2 >/dev/null &&
> +smhead1=$(cd sm2; git rev-parse --short --verify HEAD) &&

... inside its own "test_expect_success setup"?  

None of the ">/dev/null" we see in the above helper functions (and
use of these helper functions) are necessary or desired, I would
think.

The last one can become "$(git -C sm2 rev-parse ...)".

> +cd sm1

I can sort of see the attraction of doing it this way, but we avoid
chdir'ing around outside subshells, especially in a newly added test
script.  It is very easy to go down and forget to come back up, or
worse yet, stop going down and forget to remove matching "cd .." and
end up being outside the $TEST_DIRECTORY by mistake.

> +test_expect_success 'setup - submodule directory' '
> +...
> +
> +cd ..
> +
> +test_expect_success 'submodule not initialized in new clone' '
> +...

The tests themselves looked sensible.  Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 3/8] diff: prepare for additional submodule formats

[Junio C Hamano]

Jacob Keller  writes:

> +enum diff_submodule_format {
> + DIFF_SUBMODULE_SHORT = 0,
> + DIFF_SUBMODULE_LOG,
> +};

Unlike definition of an array elements, enum {} did not allow
trailing comma until recent versions of C, and we avoid it.

No need to resend only to fix this, as I have locally amended it.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] diff-highlight: add some tests.

[Brian Henderson]

On Fri, Aug 19, 2016 at 11:10:55AM -0700, Junio C Hamano wrote:
> 
> > +# vim: set noet
> 
> We tend to avoid cluttering the source with editor specific insns
> like this.

oops.

Anyone have any suggestions for project level vim settings?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git format-patch --break-rewrites broken in 2.9.3

[Andreas Schwab]

On Aug 19 2016, "Philip Oakley"  wrote:

> On Thu, Aug 18, 2016 at 04:44:21PM +0200, Olaf Hering wrote:
>
>> This command used to create a diff which can be consumed by patch. But
>> at least with 2.9.3 it just gives a rename output:
>>
>>  git format-patch \
>> --no-signature \
>> --stdout \
>> --break-rewrites \
>> --keep-subject \
>>
>> 95fa0405c5991726e06c08ffcd8ff872f7fb4f2d^..95fa0405c5991726e06c08ffcd8ff872f7fb4f2d
>>
>>
>> What must be done now to get a usable patch?
>
> As an aside, the range can be shortened to
>
> 95fa0405c5991726e06c08ffcd8ff872f7fb4f2d^!

In the context of format-patch you can also use -1 to select the topmost
commit from the list.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/3] diff-highlight: add support for --graph output.

[Junio C Hamano]

Brian Henderson  writes:

> diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh 
> b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> index 3b3c831..b88174e 100755
> --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> @@ -185,7 +185,7 @@ test_expect_failure 'diff-highlight highlights mismatched 
> hunk size' '
>  
>  # TODO add multi-byte test
>  
> -test_expect_failure 'diff-highlight works with the --graph option' '
> +test_expect_success 'diff-highlight works with the --graph option' '
>   dh_test_setup_history \
>   "aaa\nbbb\nccc\n" \
>   "aaa\n0bb\nccc\n" \

Yup, the standard "describe what we want and document a bug to be
fixed with _failure, and flip it to _success when you fix the bug"
pattern makes it very easy to understand what is going on.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] diff-highlight: add failing test for handling --graph output.

[Junio C Hamano]

Brian Henderson  writes:

> Signed-off-by: Brian Henderson 
> ---
>  contrib/diff-highlight/t/t9400-diff-highlight.sh | 54 
> 
>  1 file changed, 54 insertions(+)
>
> diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh 
> b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> index 6b8a461..3b3c831 100755
> --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
> @@ -85,10 +85,50 @@ dh_commit_test () {
>   test_cmp commit.exp commit.act
>  }
>  
> +# dh_test_setup_history takes a series (3) of changes and generates a 
> contrived graph
> +# of commits and merges

The first line look a bit overlong.

If the graph is truly "contrived" as the comment at the beginning
says (I haven't formed an opinion), it probably is a good idea to
draw a picture upfront.

> +dh_test_setup_history () {
> + a="$1" b="$2" c="$3"

Even though assignment would not fail, it probably is a good idea to
&&-chain this line, too, for consistency.  I.e.

a=$1 b=$2 c=$3 &&

Note that RHS of an assignment do not need dq; it does not hurt to
quote them, though.

> + printf "$a" >file &&
> + git add file &&
> + git commit -m"Add a file" &&

Avoid sticking a short-option to its argument, i.e.

git commit -m "Add a file"

> + printf "$b" >file &&
> + git commit -am"Update a file" &&

Likewise.

git commit -a -m "Update a file".

> +left_trim () {
> + "$PERL_PATH" -pe 's/^\s+//'
> +}
> +
> +trim_graph_el () {

What does "el" stand for?  Not E-lisp ;-)

If you meant "elements" or something, probably that is unnecessary
and "trim_graph" would be sufficient, but you may have meant
something else.

> + # graphs start with * or |
> + # followed by a space or / or \
> + "$PERL_PATH" -pe 's@^((\*|\|)( |/|\\))+@@'
> +}
> +
>  test_expect_success 'diff-highlight highlights the beginning of a line' '
>   dh_test \
>   "aaa\nbbb\nccc\n" \
> @@ -145,6 +185,20 @@ test_expect_failure 'diff-highlight highlights 
> mismatched hunk size' '
>  
>  # TODO add multi-byte test
>  
> +test_expect_failure 'diff-highlight works with the --graph option' '
> + dh_test_setup_history \
> + "aaa\nbbb\nccc\n" \
> + "aaa\n0bb\nccc\n" \
> + "aaa\nb0b\nccc\n" &&
> +
> + # topo-order so that the order of the commits is the same as with 
> --graph
> + # trim graph elements so we can do a diff
> + # trim leading space because our trim_graph_el is not perfect
> + git log -p --topo-order | "$DIFF_HIGHLIGHT" | left_trim >graph.exp &&
> + git log -p --graph | "$DIFF_HIGHLIGHT" | trim_graph_el | left_trim 
> >graph.act &&
> + test_cmp graph.exp graph.act
> +'
> +
>  test_done
>  
>  # vim: set noet
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] diff-highlight: add some tests.

[Junio C Hamano]

> +# dh_test is a test helper function which takes 1) some file data, 2) some
> +# change of the file data, creates a diff and commit of the changes and 
> passes
> +# that through diff-highlight. The optional 3rd parameter is the expected
> +# output of diff-highlight minus the diff/commit header. Don't include a 3rd
> +# parameter if diff-highlight is stupposed to leave the input unmodified.

Good explanation, but it fails to say one crucial thing.  The third
parameter is directly fed to printf and not to "printf '%s' $3",
hence any '%' you expect to see in diff output need to be doubled to
protect it.

> +# see dh_test for usage
> +dh_diff_test () {
> + a="$1" b="$2"
> +
> + printf "$a" >file
> + git add file
> +
> + printf "$b" >file
> + git diff file >diff.raw
> +
> + if test $# -ge 3
> + then
> + # Add the diff header to the expected file
> + # we remove the trailing newline to make the test a little more 
> readable
> + # this means $3 should start with a newline
> + head -n5 diff.raw | test_chomp_eof >diff.exp
> + printf "$3" >>diff.exp
> + else
> + cat diff.raw >diff.exp
> + fi

Sorry, but I do not understand why you hardcode -n5 or why you need
chomp-eof.

I _think_ you are expecting "git diff file" to start with

diff --git a/file b/file
index fff..fff 100644
--- a/file
+++ b/file
@@ -l,k +m,n @@ comments

and want to grab everything before and including this first hunk
header.  A more future-proof way to do the "stop at the first
occurrence of this" (this comment applies to -n11 we see below) is

sed -e '/^@@/q' diff.raw

As to chomp-eof, I still do not see the point, especially with the
new comment you added: "this means $3 should start with newline".

You are forcing the caller to have an extra empty line at the
beginnig ONLY because you do this "chomp" thing.  Otherwise the
callers do not need to.

Unless you actually mean something else by the new comment, e.g. "I
think the callers look prettier if it begins with a newline, so we
compensate for that by removing the end of line from what comes
before it", that is.  If "this means $3 should" is what it sounds
like, i.e. imposing an unnatural constraint on the callers of this
helper function, then the helper can do this

printf "\n$3" >>diff.exp

so that the callers do not have to worry about it.

If "I think the caller looks prettier if it begins with a newline"
is the true motivation, then "this means $3 should start..." needs
to be rephrased.  And as to the implementation of the helper, I
think

{
sed -e '/^@@/q' diff.raw
printf "$3" | sed -e 1d
} >diff.expected

may be easier to see what is going on.

> +
> + "$DIFF_HIGHLIGHT" diff.act &&

> + # check that at least one of the files is not empty (any of the above
> + # commands could have failed resulting in an empty file)
> + test -s diff.act &&

A more established way in our test suite to ensure that "any of the
above commands could have failed" is caught is to &&-chain all the
commands, like this:

a=$1 b=$2 &&
printf "$a" >file && git add file &&
printf "$b" >file && git diff file >diff.raw &&
if test $# = 3
then
...
else
...
fi &&

> +test_done
> +
> +# vim: set noet

We tend to avoid cluttering the source with editor specific insns
like this.

Stepping back a bit.  Because you are only interested in making sure
the body of the diff match what is expected, it may be a better
alternative approach to make the comparison _after_ stripping the
headers from the actual output with the expected (which you do not
have headers for), rather than comparing the expected (with fake
headers added in as necessary) and the actual output with headers,
i.e.

git diff file >diff.raw &&
if test $# = 3
then
printf "$3"
else
sed -e '1,/^@@/d' diff.expected &&
"$DIFF_HIGHLIGHT" diff.highlight &&
sed -e '1,/^@@/d' diff.actual &&
test_cmp diff.expected diff.actual

or something like that.

That does not address "is it a good idea to require an empty line at
the beginning of $3"? at all, though.  If you want to require a
blank in front of "$3", the "printf" needs to be tweaked to somehow
strip it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git format-patch --break-rewrites broken in 2.9.3

[Philip Oakley]

On Thu, Aug 18, 2016 at 04:44:21PM +0200, Olaf Hering wrote:


This command used to create a diff which can be consumed by patch. But
at least with 2.9.3 it just gives a rename output:

 git format-patch \
--no-signature \
--stdout \
--break-rewrites \
--keep-subject \

95fa0405c5991726e06c08ffcd8ff872f7fb4f2d^..95fa0405c5991726e06c08ffcd8ff872f7fb4f2d


What must be done now to get a usable patch?


As an aside, the range can be shortened to

95fa0405c5991726e06c08ffcd8ff872f7fb4f2d^!

It's something I picked up when doing the doc update on 'specifying 
revisions'.


--

Philip

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/3] diff-highlight: add support for --graph output.

[Brian Henderson]

Signed-off-by: Brian Henderson 
---
 contrib/diff-highlight/diff-highlight| 19 +--
 contrib/diff-highlight/t/t9400-diff-highlight.sh |  2 +-
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/contrib/diff-highlight/diff-highlight 
b/contrib/diff-highlight/diff-highlight
index ffefc31..9280c88 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -21,6 +21,10 @@ my $RESET = "\x1b[m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
 my $BORING = qr/$COLOR|\s/;
 
+# The patch portion of git log -p --graph should only ever have preceding | and
+# not / or \ as merge history only shows up on the commit line.
+my $GRAPH = qr/$COLOR?\|$COLOR?\s+/;
+
 my @removed;
 my @added;
 my $in_hunk;
@@ -32,12 +36,12 @@ $SIG{PIPE} = 'DEFAULT';
 while (<>) {
if (!$in_hunk) {
print;
-   $in_hunk = /^$COLOR*\@/;
+   $in_hunk = /^$GRAPH*$COLOR*\@/;
}
-   elsif (/^$COLOR*-/) {
+   elsif (/^$GRAPH*$COLOR*-/) {
push @removed, $_;
}
-   elsif (/^$COLOR*\+/) {
+   elsif (/^$GRAPH*$COLOR*\+/) {
push @added, $_;
}
else {
@@ -46,7 +50,7 @@ while (<>) {
@added = ();
 
print;
-   $in_hunk = /^$COLOR*[\@ ]/;
+   $in_hunk = /^$GRAPH*$COLOR*[\@ ]/;
}
 
# Most of the time there is enough output to keep things streaming,
@@ -163,6 +167,9 @@ sub highlight_pair {
}
 }
 
+# we split either by $COLOR or by character. This has the side effect of
+# leaving in graph cruft. It works because the graph cruft does not contain "-"
+# or "+"
 sub split_line {
local $_ = shift;
return utf8::decode($_) ?
@@ -211,8 +218,8 @@ sub is_pair_interesting {
my $suffix_a = join('', @$a[($sa+1)..$#$a]);
my $suffix_b = join('', @$b[($sb+1)..$#$b]);
 
-   return $prefix_a !~ /^$COLOR*-$BORING*$/ ||
-  $prefix_b !~ /^$COLOR*\+$BORING*$/ ||
+   return $prefix_a !~ /^$GRAPH*$COLOR*-$BORING*$/ ||
+  $prefix_b !~ /^$GRAPH*$COLOR*\+$BORING*$/ ||
   $suffix_a !~ /^$BORING*$/ ||
   $suffix_b !~ /^$BORING*$/;
 }
diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh 
b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 3b3c831..b88174e 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -185,7 +185,7 @@ test_expect_failure 'diff-highlight highlights mismatched 
hunk size' '
 
 # TODO add multi-byte test
 
-test_expect_failure 'diff-highlight works with the --graph option' '
+test_expect_success 'diff-highlight works with the --graph option' '
dh_test_setup_history \
"aaa\nbbb\nccc\n" \
"aaa\n0bb\nccc\n" \
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] diff-highlight: add failing test for handling --graph output.

[Brian Henderson]

Signed-off-by: Brian Henderson 
---
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 54 
 1 file changed, 54 insertions(+)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh 
b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 6b8a461..3b3c831 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -85,10 +85,50 @@ dh_commit_test () {
test_cmp commit.exp commit.act
 }
 
+# dh_test_setup_history takes a series (3) of changes and generates a 
contrived graph
+# of commits and merges
+dh_test_setup_history () {
+   a="$1" b="$2" c="$3"
+
+   printf "$a" >file &&
+   git add file &&
+   git commit -m"Add a file" &&
+
+   printf "$b" >file &&
+   git commit -am"Update a file" &&
+
+   git checkout -b branch &&
+   printf "$c" >file &&
+   git commit -am"Update a file on branch" &&
+
+   git checkout master &&
+   printf "$a" >file &&
+   git commit -am"Update a file again" &&
+
+   git checkout branch &&
+   printf "$b" >file &&
+   git commit -am"Update a file similar to master" &&
+
+   git merge master &&
+   git checkout master &&
+   git merge branch --no-ff
+}
+
+
 test_chomp_eof () {
"$PERL_PATH" -pe 'chomp if eof'
 }
 
+left_trim () {
+   "$PERL_PATH" -pe 's/^\s+//'
+}
+
+trim_graph_el () {
+   # graphs start with * or |
+   # followed by a space or / or \
+   "$PERL_PATH" -pe 's@^((\*|\|)( |/|\\))+@@'
+}
+
 test_expect_success 'diff-highlight highlights the beginning of a line' '
dh_test \
"aaa\nbbb\nccc\n" \
@@ -145,6 +185,20 @@ test_expect_failure 'diff-highlight highlights mismatched 
hunk size' '
 
 # TODO add multi-byte test
 
+test_expect_failure 'diff-highlight works with the --graph option' '
+   dh_test_setup_history \
+   "aaa\nbbb\nccc\n" \
+   "aaa\n0bb\nccc\n" \
+   "aaa\nb0b\nccc\n" &&
+
+   # topo-order so that the order of the commits is the same as with 
--graph
+   # trim graph elements so we can do a diff
+   # trim leading space because our trim_graph_el is not perfect
+   git log -p --topo-order | "$DIFF_HIGHLIGHT" | left_trim >graph.exp &&
+   git log -p --graph | "$DIFF_HIGHLIGHT" | trim_graph_el | left_trim 
>graph.act &&
+   test_cmp graph.exp graph.act
+'
+
 test_done
 
 # vim: set noet
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 0/3] diff-highlight: add support for git log --graph output.

[Brian Henderson]

I cleaned up the graph test, hopefully it's better.

Brian Henderson (3):
  diff-highlight: add some tests.
  diff-highlight: add failing test for handling --graph output.
  diff-highlight: add support for --graph output.

 contrib/diff-highlight/Makefile  |   5 +
 contrib/diff-highlight/diff-highlight|  19 ++-
 contrib/diff-highlight/t/Makefile|  22 +++
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 204 +++
 4 files changed, 244 insertions(+), 6 deletions(-)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh

-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/3] diff-highlight: add some tests.

[Brian Henderson]

Signed-off-by: Brian Henderson 
---
 contrib/diff-highlight/Makefile  |   5 +
 contrib/diff-highlight/t/Makefile|  22 
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 150 +++
 3 files changed, 177 insertions(+)
 create mode 100644 contrib/diff-highlight/Makefile
 create mode 100644 contrib/diff-highlight/t/Makefile
 create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh

diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
new file mode 100644
index 000..9018724
--- /dev/null
+++ b/contrib/diff-highlight/Makefile
@@ -0,0 +1,5 @@
+# nothing to build
+all:
+
+test:
+   $(MAKE) -C t
diff --git a/contrib/diff-highlight/t/Makefile 
b/contrib/diff-highlight/t/Makefile
new file mode 100644
index 000..5ff5275
--- /dev/null
+++ b/contrib/diff-highlight/t/Makefile
@@ -0,0 +1,22 @@
+-include ../../../config.mak.autogen
+-include ../../../config.mak
+
+# copied from ../../t/Makefile
+SHELL_PATH ?= $(SHELL)
+SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
+
+all: test
+test: $(T)
+
+.PHONY: help clean all test $(T)
+
+help:
+   @echo 'Run "$(MAKE) test" to launch test scripts'
+   @echo 'Run "$(MAKE) clean" to remove trash folders'
+
+$(T):
+   @echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+
+clean:
+   $(RM) -r 'trash directory'.*
diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh 
b/contrib/diff-highlight/t/t9400-diff-highlight.sh
new file mode 100755
index 000..6b8a461
--- /dev/null
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -0,0 +1,150 @@
+#!/bin/sh
+
+test_description='Test diff-highlight'
+
+CURR_DIR=$(pwd)
+TEST_OUTPUT_DIRECTORY=$(pwd)
+TEST_DIRECTORY="$CURR_DIR"/../../../t
+DIFF_HIGHLIGHT="$CURR_DIR"/../diff-highlight
+
+CW="\033[7m"   # white
+CR="\033[27m" # reset
+
+. "$TEST_DIRECTORY"/test-lib.sh
+
+if ! test_have_prereq PERL
+then
+   skip_all='skipping diff-highlight tests; perl not available'
+   test_done
+fi
+
+# dh_test is a test helper function which takes 1) some file data, 2) some
+# change of the file data, creates a diff and commit of the changes and passes
+# that through diff-highlight. The optional 3rd parameter is the expected
+# output of diff-highlight minus the diff/commit header. Don't include a 3rd
+# parameter if diff-highlight is stupposed to leave the input unmodified.
+dh_test () {
+   dh_diff_test "$@" &&
+   dh_commit_test "$@"
+}
+
+# see dh_test for usage
+dh_diff_test () {
+   a="$1" b="$2"
+
+   printf "$a" >file
+   git add file
+
+   printf "$b" >file
+   git diff file >diff.raw
+
+   if test $# -ge 3
+   then
+   # Add the diff header to the expected file
+   # we remove the trailing newline to make the test a little more 
readable
+   # this means $3 should start with a newline
+   head -n5 diff.raw | test_chomp_eof >diff.exp
+   printf "$3" >>diff.exp
+   else
+   cat diff.raw >diff.exp
+   fi
+
+   "$DIFF_HIGHLIGHT" diff.act &&
+   # check that at least one of the files is not empty (any of the above
+   # commands could have failed resulting in an empty file)
+   test -s diff.act &&
+   test_cmp diff.exp diff.act
+}
+
+# see dh_test for usage
+dh_commit_test () {
+   a="$1" b="$2"
+
+   printf "$a" >file
+   git add file
+   git commit -m "Add a file" >/dev/null
+
+   printf "$b" >file
+   git commit -am "Update a file" >/dev/null
+
+   git show >commit.raw
+
+   if test $# -ge 3
+   then
+   # same as dh_diff_test
+   head -n11 commit.raw | test_chomp_eof >commit.exp
+   printf "$3" >>commit.exp
+   else
+   cat commit.raw >commit.exp
+   fi
+
+   "$DIFF_HIGHLIGHT" commit.act &&
+   # check that at least one of the files is not empty (any of the above
+   # commands could have failed resulting in an empty file)
+   test -s commit.act &&
+   test_cmp commit.exp commit.act
+}
+
+test_chomp_eof () {
+   "$PERL_PATH" -pe 'chomp if eof'
+}
+
+test_expect_success 'diff-highlight highlights the beginning of a line' '
+   dh_test \
+   "aaa\nbbb\nccc\n" \
+   "aaa\n0bb\nccc\n" \
+"
+ aaa
+-${CW}b${CR}bb
++${CW}0${CR}bb
+ ccc
+"
+'
+
+test_expect_success 'diff-highlight highlights the end of a line' '
+   dh_test \
+   "aaa\nbbb\nccc\n" \
+   "aaa\nbb0\nccc\n" \
+"
+ aaa
+-bb${CW}b${CR}
++bb${CW}0${CR}
+ ccc
+"
+'
+
+test_expect_success 'diff-highlight highlights the middle of a line' '
+   dh_test \
+   "aaa\nbbb\nccc\n" \
+   "aaa\nb0b\nccc\n" \
+"
+ aaa
+-b${CW}b${CR}b
++b${CW}0${CR}b
+ ccc
+"
+'
+
+test_expect_success 'diff-highlight does not highlight whole line' '
+   dh_test \
+   

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Stefan Beller]

It was not my intend to start this discussion again with my initial email.
I rather wanted to point out how I make progress in doing my own
tooling.

I mean if email works well for Junio (both as a maintainer as
well as a contributor) and Jeff as a contributor, then I can adapt
my workflow to that, because these two have brought in
8300 of 33000 non merge patches. (i.e. they had 25% of the
patches over the life time of the project and are with the project
longer than others IIUC). So why would I demand they change
their style just to accommodate one newcomer like me?

>
>> I see a choice of mail client as no different than a choice of
>> text editor.  Neither my mail client or text editor is heavily
>> customized.  The key feature I rely on from both tools is piping
>> data to external commands.
>
> There you go. That key feature seems to be unavailable in the most
> wide-spread email client: Outlook. So by not restricting the choice you
> should make it possible to use that mail client, too, right?

Well I think this data piping is essential to any workflow. Even if were to
abandon email completely and roll our own communications protocol,
one of the first things added would be an API to "use your own text editor".

In my case git-send-email works well for the sending direction without a lot
of custom tooling (read: none apart from the initial configuration).

>
> We do not even have a section on Outlook in our SubmittingPatches.

"You can write one? Pretty please?"
would be the canonical answer. ;)

>
> Okay, if not the most popular mail client, then web mail? Nope, nope,
> nope. No piping *at all* to external commands from there.
>
> So you basically slam the door shut on the vast majority of email users.
>
> That is not leaving much choice to the users in my book.
>
>> OTOH, today, I see people using git aliases all the time which
>> look more like ASM instructions than user commands.
>
> I see this as a completely different beast. Aliases help users accelerate
> their personal workflow. Whereas anybody who is already willing to
> contribute to Git *must* go through that non-personal workflow we impose:
> paste the diff in a very specific format into the mail, and don't you dare
> use a mail client that mangles whitespace (which is, like, pretty much
> every single popular mail client out there).

Maybe we should invent a patch format that copes with broken whitespace?
(git-format-patch --allow-broken-whitespace), e.g. replace a tab by
"-___" (not exactly 8 chars, but stopping at the columns 8, 16, 24 etc)
git am/apply would need to know about the unbreaking the white space, too.

>
> So *allowing* users to configure their own aliases, and *forcing* them to
> figure out how to transport patches through a medium hostile to patches,
> is pretty much two diametrically opposed things.

I think the point was that people carelessly expose their aliases in e.g. their
blogs. (e.g. "to do  you just need to git a  & ci &&
git rr", which
leaves others just guessing? It certainly feels similar to not knowing how to
configure your email client?)

>
>> Users ought to be able to pick, choose, and replace tools as
>> they wish as long as an interchange format remains stable
>> and widely-supported.
>
> Right. Let's talk about the interchange format called mails, for the data
> called patches. Is it stable and widely-supported?

It is stable as it has been around for years and you can choose whether you
use git apply or the patch utility. It is widely supported as it is raw text so
it can be used across different platforms. However it doesn't cope well with
email, as email modifies text sometimes such as mangling white spaces.

>
> Can users really pick and choose the tools they like most to send patches
> to the Git project? Like, the Outlook client? Or the GMail client?

Of course, see[1] ;)
[1] 
https://public-inbox.org/git/CA+55aFy2AEe7ew5Px=2Uit6hraGV9zFr=jz57rsyxwmq4nm...@mail.gmail.com/

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 0/1] Rename NotNormalized (NNO) into CRLF in index

[Junio C Hamano]

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
> Here comes the promised cleanup of t0027:
> - The wording NNO is removed and replaced by CRI
> - No code changes
> - Needs to go on top of next or pu or tb/t0027-raciness-fix
> Torsten Bögershausen (1):
>   t0027: Rename NotNormalized (NNO) into CRLF in index
>
>  t/t0027-auto-crlf.sh | 122 
> +--
>  1 file changed, 61 insertions(+), 61 deletions(-)

Until these acronyms are shown expanded upfront in the file (not in
the log message), this patch does not help the readers at all.  They
used to say "What the heck is NNO?"  Now they will be left wondering
"What the heck is CRI?".

Have a comment before the first use of the acronym, perhaps?

@@ -102,7 +84,7 @@ commit_check_warn () {
check_warning "$crlfnul" ${pfx}_CRLF_nul.err
 }
 
-commit_chk_wrnNNO () {
+CRI_add_chk_wrn () {
attr=$1 ; shift
aeol=$1 ; shift
crlf=$1 ; shift


Incidentally it is a good place to describe what this humongous
helper function is meant to do.

# Having CR in the indexed contents (CRI in short) poses such and
# such tricky cases; this helper tests combinations of xyzzy, frotz
# and nitfol exhaustively by running xyzzy through (a, b, c),
# flipping frotz between (x, y), and ...

or something like that (of course the blanks need to be filled),
perhaps?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

[Junio C Hamano]

Jeff King  writes:

>> Sorry, Torsten, this is not my doing. So I cannot explain why it is not an
>> enum.
>> 
>> I *guess* the rationale for 'c' being the cmdmode of --textconv is "c for
>> convert". That is why I chose "w as in worktree" as new cmdmode.
>
> I think it started as a 'char' because it was representing the
> single-letter options of "cat-file -e", "cat-file -s", etc. And then the
> textconv patches started the monstrosity of "c".

I think it was merely 't' was taken for "-t" (one beauty of using
the OPT_CMDMODE for options with shorthand is that we do not need
any enum, as the short-form options already form an enum), so
"textConv" was the compromise.  It could have been 'T' or even \C-t
;-)

> I don't think cleaning it up needs to be part of your series, but it
> might be nice low-hanging fruit for somebody to clean up on top.

I agree.

I briefly wondered if we want to add a check to ensure uniqueness of
these cmdmode letters in parse_options_check(), but that would
forbid us from having synonymous options (e.g. "am --continue" and
"am --resolved" both map to the same), so it would not work.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Johannes Schindelin  writes:

>> I think I saw some code to ensure "when giving this option you need
>> that option in effect, too"; they should be tested here, too, no?
>
> No, I would rather not test for that. These conditionals are purely for
> any user's convenience, in case they specify an option that has no effect.
> They are absolutely not essential for the function introduced in this
> patch series.

I didn't say "you would want to test these, no?", did I?

I do not want to see bugreports that say "I wanted to use this new
feature and by mistake gave only --path without giving --filter; Git
should have complained.  I found a bug, hooray!" when somebody in
the future refactors the command line option parsing and breaks the
check you already have.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Johannes Schindelin  writes:

>> Does it make sense to be more specific here:
>> The order of conversion is
>> - ident
>> - CRLF
>> - smudge
>
> I do not think it makes sense to complexify the documentation in that
> manner.

Right.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Johannes Schindelin  writes:

> Actually, I find it much harder to debug these "the output must match
> these precise bytes, else we fail" type of test cases. Instead, I describe
> in a natural way what is expected:
>
>   ! has_cr actual
>
> Now, when the test fails, whoever is that poor soul tasked with debugging
> and fixing the breakage knows *what* goes wrong, conceptually.

I am of two minds but 60% in favor of the "We only care about
presense of CR" approach.  Of course, the remaining 40% is "other
people may break the code in such a way that still has CR at the end
of the line but change ealier bytes on the line in the future (an
obvious way to do so is to turn an input "ABC\n" into "AB\r\n"), and
we would want to catch it".  But the conversion machinery is tested
elsewhere in the test suite, and this test is more about the cat-file
command making a call into the conversion machinery when and only
when it is necessary, so that sways me towards "has_cr is what we
want here".

> Could you please start surrounding your replies by empty lines?

Good suggestion.  I have exactly the same problem whenever I read
messages without blanks at paragraph boundaries.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mingw: ensure temporary file handles are not inherited by child processes

[Junio C Hamano]

Eric Wong  writes:

> I'd be more comfortable keeping the EINVAL check that got
> snipped in your reply.  O_CLOEXEC can be defined to non-zero in
> new userspace headers, but an older kernel chokes on it with
> EINVAL.

Good point.  Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] help: introduce option --command-only

[Junio C Hamano]

Johannes Schindelin  writes:

> - configure help.format = html (for "man", the current code would always
>   add $(prefix)/share/man to the MANPATH when testing, not what we want,
>   and hacking this code *just* for testing is both ugly and unnecessary).

A very constructive suggestion to show a good direction.  Because we
are not in the business of verifying the "man" works as expected and
how it wants the files in MANPATH are structured, it is a very good
idea to do our testing with the HTML format, which gives us tighter
control of what we actually use for our testing with help.browser
configuration variable.  I really like it.

> - configure help.htmlpath to point to a subdirectory that is created and
>   populated in the same test script.

Yup!

> - configure help.browser to point to a script that is created in the same
>   script and whose output we can verify, too.

Yup, yup!  The "browser" can be something that parrots its command
line to the standard output and does not even have to care if the
file pointed at is HTML at all.

> The last point actually requires a patch that was recently introduced into
> Git for Windows [*1*] (and that did not make it upstream yet) which
> reverts that change whereby web--browse was sidestepped. That sidestepping
> was well-intentioned but turned out to cause more harm than good.

Good.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Johannes Schindelin]

Hi Eric,

On Thu, 18 Aug 2016, Eric Wong wrote:

> Johannes Schindelin  wrote:
>
> > Old dogs claim the mail list-approach works for them. Nope. Doesn't.
> > Else you would not have written all those custom scripts.
> 
> git and cogito started as a bunch of custom scripts, too.

The difference is that neither git nor cogito were opinionated. Those
custom scripts are. They are for one particular workflow, with one
particular mail client, with a strong bias to a Unix-y environment.

I work really hard to make Git for Windows as easy and fun to use as
possible. I just wish that we were working together to make it as easy and
fun to contribute to Git, too.

> I see a choice of mail client as no different than a choice of
> text editor.  Neither my mail client or text editor is heavily
> customized.  The key feature I rely on from both tools is piping
> data to external commands.

There you go. That key feature seems to be unavailable in the most
wide-spread email client: Outlook. So by not restricting the choice you
should make it possible to use that mail client, too, right?

We do not even have a section on Outlook in our SubmittingPatches.

Okay, if not the most popular mail client, then web mail? Nope, nope,
nope. No piping *at all* to external commands from there.

So you basically slam the door shut on the vast majority of email users.

That is not leaving much choice to the users in my book.

> OTOH, today, I see people using git aliases all the time which
> look more like ASM instructions than user commands.

I see this as a completely different beast. Aliases help users accelerate
their personal workflow. Whereas anybody who is already willing to
contribute to Git *must* go through that non-personal workflow we impose:
paste the diff in a very specific format into the mail, and don't you dare
use a mail client that mangles whitespace (which is, like, pretty much
every single popular mail client out there).

So *allowing* users to configure their own aliases, and *forcing* them to
figure out how to transport patches through a medium hostile to patches,
is pretty much two diametrically opposed things.

> Users ought to be able to pick, choose, and replace tools as
> they wish as long as an interchange format remains stable
> and widely-supported.

Right. Let's talk about the interchange format called mails, for the data
called patches. Is it stable and widely-supported?

Can users really pick and choose the tools they like most to send patches
to the Git project? Like, the Outlook client? Or the GMail client?

> Even today, at least one Linux kernel hacker still uses quilt to
> generate patches: http://ozlabs.org/~akpm/mmotm/

Andrew does not count, he lives in his own universe.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Johannes Schindelin]

Hi,

On Thu, 18 Aug 2016, Junio C Hamano wrote:

> Eric Wong  writes:
> 
> > unsubscribe: meta+unsubscr...@public-inbox.org
> 
> Did you mean this, really?

FWIW I do not see this line in my original mail from Eric.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] diff-highlight: add some tests.

[Brian Henderson]

On Fri, Aug 19, 2016 at 10:51:23AM -0400, Jeff King wrote:
> On Fri, Aug 19, 2016 at 07:42:35AM -0700, Brian Henderson wrote:
> 
> > > > +# PERL is required, but assumed to be present, although not 
> > > > necessarily modern
> > > > +# some tests require 5.8
> > > > +test_expect_success PERL 'name' 'true'
> > > 
> > > If the platform lacks PERL prerequisite, this will simply be
> > > skipped, and if the platform has it, it will always succeed.
> > > 
> > > I am not sure what you are trying to achieve by having this line
> > > here.
> > 
> > I originally didn't have this line, and my comment was referring to the
> > t/README which says
> > 
> >Even without the PERL prerequisite, tests can assume there is a
> >usable perl interpreter at $PERL_PATH, though it need not be
> >particularly modern.
> > 
> > There is current functionality in diff-highlight which requires at least
> > perl 5.8 (the utf8 functions). I was going to add a test for this as
> > well, but I'm not super comfy with multibyte chars.
> 
> Yeah, I'd agree this test would want the PERL prereq. It is not just
> using perl for one-liners in support of the script; it is testing major
> perl functionality that should be skipped if we do not have a modern
> perl available.
> 
> > Eric recommended adding this line, what do you think?
> > 
> > would `test_set_prereq PERL` be better?
> 
> test_set_prereq is for telling the test scripts that we _have_ perl, but
> what I think this script wants to do is test "do we have perl?" and
> abort otherwise. The way to do that is:
> 
>   if ! test_have_prereq PERL
>   then
>   skip_all='skipping diff-highlight tests; perl not available'
>   test_done
>   fi
> 
> > > > +test_expect_success 'diff-highlight does not highlight whole line' '
> > > > +   dh_test \
> > > > +   "aaa\nbbb\nccc\n" \
> > > > +   "aaa\n000\nccc\n"
> > > > +'
> > 
> > This (at least to me) is desired. See comment for `sub
> > is_pair_interesting`
> 
> Yeah, that is an intentional behavior, and makes sense to test.
> 
> > > > +test_expect_success 'diff-highlight does not highlight mismatched hunk 
> > > > size' '
> > > > +   dh_test \
> > > > +   "aaa\nbbb\n" \
> > > > +   "aaa\nb0b\nccc\n"
> > > > +'
> > 
> > This is undesired behavior, but currently implemented for simplicity,
> > see `sub show_hunk`
> > 
> > Do they need comments or something?
> 
> Undesired behavior should generally not be tested for. It just makes
> life harder for somebody when they make a change that violates it, and
> they have to figure out "oh, but it's _good_ that I changed that, the
> tests were wrong" (or more likely "I didn't fix it, but it's just broken
> in a different way, and neither is preferable").
> 
> If you want to document known shortcomings, the best thing to do is show
> what you'd _like_ to have happen, and mark it as test_expect_failure;
> the test scripts show this as a known-breakage, and somebody later who
> fixes it can flip the "failure" to "success".
> 
> -Peff

all very helpful, thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

[Jeff King]

On Fri, Aug 19, 2016 at 02:25:51PM +0200, Johannes Schindelin wrote:

> > On Thu, Aug 18, 2016 at 02:46:28PM +0200, Johannes Schindelin wrote:
> > > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > > index 5ff58b3..5f91cf4 100644
> > > --- a/builtin/cat-file.c
> > > +++ b/builtin/cat-file.c
> > > @@ -17,6 +17,7 @@ struct batch_options {
> > >   int print_contents;
> > >   int buffer_output;
> > >   int all_objects;
> > > + int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
> > How do I read 'w' and 'c' ?
> > wilter and cextconv ? Does it make sense to use an enum here ?
> > Or a #define ?
> 
> Sorry, Torsten, this is not my doing. So I cannot explain why it is not an
> enum.
> 
> I *guess* the rationale for 'c' being the cmdmode of --textconv is "c for
> convert". That is why I chose "w as in worktree" as new cmdmode.

I think it started as a 'char' because it was representing the
single-letter options of "cat-file -e", "cat-file -s", etc. And then the
textconv patches started the monstrosity of "c".

I don't think cleaning it up needs to be part of your series, but it
might be nice low-hanging fruit for somebody to clean up on top.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Jeff King]

On Thu, Aug 18, 2016 at 02:42:34PM +0200, Johannes Schindelin wrote:

> BTW I take this thread as yet another proof that people are unhappy with
> mail list-based review: if you have to build *that much* tooling around it
> (and Peff & Junio certainly have a megaton of advanced and sophisticated
> tooling around it, holy cow!) it is really incorrect to state that the
> mail list-driven approach works for you. It is much closer to the truth to
> say that the mail-list-plus-loads-of-custom-tools-driven approach works
> for you.
> 
> I am really not a fan of this.
> 
> The theory "it's inclusive because everyone has access to mail" falls on
> its face, badly, when even old timers have to build entire infrastructures
> around it just to begin to be able to use it efficiently.
> 
> It reminds me of an old software developer I met long ago, who claimed CVS
> works for him. He had written tens of thousands of lines of shell scripts,
> is what allowed "CVS" to work for him.
> 
> Same here. Old dogs claim the mail list-approach works for them. Nope.
> Doesn't. Else you would not have written all those custom scripts.

I read this over, didn't agree, waited a whole day for perspective, and
still just can't agree. So now I'm responding. :)

There is nothing wrong with building tooling around your workflow. If we
had a GitHub-based workflow, I'd build tooling around that, too. One of
the things I _like_ about a mail-based workflow is how easy it is to
build that tooling, and to get it to integrate with other existing
tools. It's the major reason I'm resistant to moving development to
GitHub. Not only would I have to throw away all of my tools, but I'm not
sure I could easily build equivalent ones.

Now, I am perfectly open to the idea that more of the tooling should be
standardized, so people do not have to build their own. But the major
problem there is that so much of what I've built is about gluing things
together for the rest of _my_ tools. I've shared my techniques and
scripts for using mutt several times, but they don't do somebody a lick
of good if they are using gmail or thunderbird.

So I don't really think I have a megaton of tooling. I just have a
little bit of glue to existing tools I was using anyway. And I think
that is where the disconnect is. If you are not using mutt already, then
it sure seems like a big jump to change your MUA. And I'm sympathetic to
that. But I don't think that means that the mailing-list approach is not
working for me, as you claim in the last paragraph.

-Peff

PS There _are_ some open questions in our workflow that are not really
   mailing list specific. E.g., the fact that it is hard to see whether
   and if your patch was picked up by Junio, what changes were made,
   tracking multiple versions, etc. I _don't_ have good tooling for
   that, but it's something that could be made generally available, as
   it's unrelated to the MUA. It's also not necessarily specific to
   mailing list development (e.g., a push-based workflow that
   aggressively rebases runs into the same versioning questions).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johannes Schindelin]

Hi Torsten,

On Fri, 19 Aug 2016, Torsten Bögershausen wrote:

> On Thu, Aug 18, 2016 at 02:46:17PM +0200, Johannes Schindelin wrote:
> 
> > +--filters::
> > +   Show the content as transformed by the filters configured in
>
> Minor comment:
> s/transformed/converted/ ?

Sure.

> Does it make sense to be more specific here:
> The order of conversion is
> - ident
> - CRLF
> - smudge

I do not think it makes sense to complexify the documentation in that
manner. The filters should always be applied in the same order, methinks,
and it would only clutter the man page to repeat that order here.

Ciao,
Dscho

Re: [PATCH v2 1/3] diff-highlight: add some tests.

[Jeff King]

On Fri, Aug 19, 2016 at 07:42:35AM -0700, Brian Henderson wrote:

> > > +# PERL is required, but assumed to be present, although not necessarily 
> > > modern
> > > +# some tests require 5.8
> > > +test_expect_success PERL 'name' 'true'
> > 
> > If the platform lacks PERL prerequisite, this will simply be
> > skipped, and if the platform has it, it will always succeed.
> > 
> > I am not sure what you are trying to achieve by having this line
> > here.
> 
> I originally didn't have this line, and my comment was referring to the
> t/README which says
> 
>Even without the PERL prerequisite, tests can assume there is a
>usable perl interpreter at $PERL_PATH, though it need not be
>particularly modern.
> 
> There is current functionality in diff-highlight which requires at least
> perl 5.8 (the utf8 functions). I was going to add a test for this as
> well, but I'm not super comfy with multibyte chars.

Yeah, I'd agree this test would want the PERL prereq. It is not just
using perl for one-liners in support of the script; it is testing major
perl functionality that should be skipped if we do not have a modern
perl available.

> Eric recommended adding this line, what do you think?
> 
> would `test_set_prereq PERL` be better?

test_set_prereq is for telling the test scripts that we _have_ perl, but
what I think this script wants to do is test "do we have perl?" and
abort otherwise. The way to do that is:

  if ! test_have_prereq PERL
  then
skip_all='skipping diff-highlight tests; perl not available'
test_done
  fi

> > > +test_expect_success 'diff-highlight does not highlight whole line' '
> > > + dh_test \
> > > + "aaa\nbbb\nccc\n" \
> > > + "aaa\n000\nccc\n"
> > > +'
> 
> This (at least to me) is desired. See comment for `sub
> is_pair_interesting`

Yeah, that is an intentional behavior, and makes sense to test.

> > > +test_expect_success 'diff-highlight does not highlight mismatched hunk 
> > > size' '
> > > + dh_test \
> > > + "aaa\nbbb\n" \
> > > + "aaa\nb0b\nccc\n"
> > > +'
> 
> This is undesired behavior, but currently implemented for simplicity,
> see `sub show_hunk`
> 
> Do they need comments or something?

Undesired behavior should generally not be tested for. It just makes
life harder for somebody when they make a change that violates it, and
they have to figure out "oh, but it's _good_ that I changed that, the
tests were wrong" (or more likely "I didn't fix it, but it's just broken
in a different way, and neither is preferable").

If you want to document known shortcomings, the best thing to do is show
what you'd _like_ to have happen, and mark it as test_expect_failure;
the test scripts show this as a known-breakage, and somebody later who
fixes it can flip the "failure" to "success".

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johannes Schindelin]

Hi Junio,

On Thu, 18 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > There are circumstances when it is relatively easy to figure out the
> > object name for a given path, but not the revision. For example, when
> 
> revision of the containing tree, I presume?

name of containing tree, actually.

> > Let's simplify this dramatically, because we do not really need that
> > revision at all: all we care about is that we know the path. In the
> > scenario described above, we do know the path, and we just want to
> > specify it separately from the object name.
> 
> I wouldn't call it "simplifying dramatically".  It is just to
> support two use cases that is different from the use case where you
> want to use ":".

"this" was not the code in question. "this" is the use case. Just put my
shoes on for a moment. As described: you have a list of object names and
their path. Now you need to use the --textconv or --filters option, but
you do not have the commit name. What do you do? Concoct a script that
goes through `git rev-list --all -- ` in the hopes of finding a
revision soon? I hope after this little exercise you agree that
introducing the --path= option simplifies this use case
dramatically.

> We already have a precedent in "hash-object --path=" for these
> two different uses cases from the primary one.  That form can be
> used when we know the contents but we do not know where the contents
> came from.  It can also be used when we want to pretend that
> contents came from a specific path, that may be different from the
> file we are hashing.

Agreed. I was unaware of hash-object's existing option.

> Let's be consistent and (1) call it "--path", and (2) explain it as
> a feature to allow you to tell the path separately or allow you to
> pretend that the content is at a path different from reality.
> 
> After all, you would want to ignore  part in this construct:
> 
>   git cat-file --filter --path= HEAD:
> 
> for the purpose of filtering, right?

True, and I even make use of that in the test for the batch mode.

> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index 0b74afa..5ff58b3 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -20,6 +20,8 @@ struct batch_options {
> > const char *format;
> >  };
> >  
> > +static const char *force_path;
> > +
> >  static int filter_object(const char *path, unsigned mode,
> >  const unsigned char *sha1,
> >  char **buf, unsigned long *size)
> > @@ -89,21 +91,24 @@ static int cat_one_file(int opt, const char *exp_type, 
> > const char *obj_name,
> > return !has_sha1_file(sha1);
> >  
> > case 'w':
> > -   if (!obj_context.path[0])
> > +   if (!force_path && !obj_context.path[0])
> > die("git cat-file --filters %s:  must be "
> > "", obj_name);
> >  
> > -   if (filter_object(obj_context.path, obj_context.mode,
> > +   if (filter_object(force_path ? force_path : obj_context.path,
> > + force_path ? 0100644 : obj_context.mode,
> >   sha1, , ))
> 
> The mode override is somewhat questionable.  Wouldn't you want to
> reject
> 
>   git cat-file --filter --path=Makefile HEAD:RelNotes
> 
> when HEAD:RelNotes blob is known to be a symbolic link?  After all,
> you would reject this
> 
>   git cat-file --filter --path=Makefile HEAD:t/
> 
> and it is quite similar, no?  I think this can be argued both ways,
> but I have a feeling that obj_context.mode, if available, should be
> honored with or without force_path.

I see your point. All I wanted to do was to enable

git cat-file --filters --path=Makefile cafebabe

in which case obj_context.mode == S_IFINVALID. I fixed it (see interdiff
of the next iteration).

> > diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
> > index e466634..fd17159 100755
> > --- a/t/t8010-cat-file-filters.sh
> > +++ b/t/t8010-cat-file-filters.sh
> > @@ -31,4 +31,17 @@ test_expect_success 'cat-file --filters converts to 
> > worktree version' '
> > has_cr actual
> >  '
> >  
> > +test_expect_success 'cat-file --filters --use-path= works' '
> > +   sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
> > +   git cat-file --filters --use-path=world.txt $sha1 >actual &&
> > +   has_cr actual
> > +'
> > +
> > +test_expect_success 'cat-file --textconv --use-path= works' '
> > +   sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
> > +   test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
> > +   git cat-file --textconv --use-path=hello.txt $sha1 >rot13 &&
> > +   test uryyb = "$(cat rot13 | remove_cr)"
> > +'
> 
> I think I saw some code to ensure "when giving this option you need
> that option in effect, too"; they should be tested here, too, no?

No, I would rather not test for that. These conditionals are purely for
any user's 

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

[Jeff King]

On Fri, Aug 19, 2016 at 02:59:32PM +0200, Johannes Schindelin wrote:

> > The only thing that somewhat is worrysome is what would happen to
> > %(rest).
> 
> What would happen is that it would print out the path, as it is exactly
> that "rest" field that is used in the implementation, piggybacking on that
> very nice feature.

Good, that is what I would expect to happen.

> Of course, this means that you simply cannot send "SHA-1 path rest" to
> cat-file --batch and expect that to work. Should anybody need that in the
> future, they can give it a try themselves to patch cat-file.

Right. The real motivation for %(rest) was to accept "git rev-list
--objects" format, so you can do stuff like:

  git rev-list --objects --all |
  git cat-file --batch-check='%(objectsize) %(rest)' |
  sort -rn |
  head

to get the paths of the largest objects. So the fact that "%(rest)" will
be the filename in this case is almost certainly the most useful default
(and if somebody really wants something else, they can try to figure out
other semantics later).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] diff-highlight: add support for --graph output.

[Jeff King]

On Wed, Aug 17, 2016 at 08:31:24AM -0700, Brian Henderson wrote:

>  contrib/diff-highlight/diff-highlight | 16 ++--
>  1 file changed, 10 insertions(+), 6 deletions(-)

Junio already commented on the tests, and I think everything he said in
his review is sensible.

As for the code itself, this looks much simpler than some of the things
we discussed off-list, which is good. There is one place that you did
not touch that I think is interesting: split_line.

It splits on $COLOR to mark those bits in their own list elements (so they
can be skipped). But we don't do anything special for $GRAPH there.

Obviously it would be wrong to find $GRAPH in the middle of the line.
But I think we end up in highlight_pair() with a sequence like this for
each line:

  [
color-graph,
"|"
color-reset,
" ",
... possibly more graph pipes ...
"-" (or "+")
... actual line data ...
  ]

We ignore "$COLOR" in the middle of that list, but not the other
syntactic bits. I think this just works because we have to skip forward
to the "-" or "+" element, and that happens to skip over all of the
graph cruft, as well.

In a more general sense, it might have been better for split_line() to
return a list of records, with each one containing a bit for "am I
interesting?" along with the actual token data. But I think because the
graph data cannot contain "-" or "+", it's acceptable to simply leave
this as it is. It might be worth a comment (either in-code, or
describing the strategy in the commit message).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] diff-highlight: add some tests.

[Brian Henderson]

On Wed, Aug 17, 2016 at 12:09:25PM -0700, Junio C Hamano wrote:
> Brian Henderson  writes:



> 
> > +
> > +# PERL is required, but assumed to be present, although not necessarily 
> > modern
> > +# some tests require 5.8
> > +test_expect_success PERL 'name' 'true'
> 
> If the platform lacks PERL prerequisite, this will simply be
> skipped, and if the platform has it, it will always succeed.
> 
> I am not sure what you are trying to achieve by having this line
> here.

I originally didn't have this line, and my comment was referring to the
t/README which says

   Even without the PERL prerequisite, tests can assume there is a
   usable perl interpreter at $PERL_PATH, though it need not be
   particularly modern.

There is current functionality in diff-highlight which requires at least
perl 5.8 (the utf8 functions). I was going to add a test for this as
well, but I'm not super comfy with multibyte chars.

Eric recommended adding this line, what do you think?

would `test_set_prereq PERL` be better?

> 
> > +test_expect_success 'diff-highlight does not highlight whole line' '
> > +   dh_test \
> > +   "aaa\nbbb\nccc\n" \
> > +   "aaa\n000\nccc\n"
> > +'

This (at least to me) is desired. See comment for `sub
is_pair_interesting`

> 
> Hmm, does this express the desired outcome, or just document the
> current (possibly broken--I dunno) behaviour?  The same question for
> the next one.
> 
> > +test_expect_success 'diff-highlight does not highlight mismatched hunk 
> > size' '
> > +   dh_test \
> > +   "aaa\nbbb\n" \
> > +   "aaa\nb0b\nccc\n"
> > +'

This is undesired behavior, but currently implemented for simplicity,
see `sub show_hunk`

Do they need comments or something?



> 
> > +   test -s diff.act &&
> 
> Why?  If you always have the expected output that you are going to
> compare with, wouldn't that sufficient to do that test without this?
> Besides, having "test -s" means that you can never make sure that a
> certain pair of input does not show any changes.  Perhaps drop it?

I was trying to address Eric's concern for `printf` or `git commit` et
al failing. Also, this file will always be a diff, it just might not
having any highlighting (so not empty?).

I'll take another stab.

> 
> > +   diff diff.exp diff.act
> 
> Use test_cmp unless there is a strong reason why you shouldn't?
> 
> > +}
> > +
> > +dh_commit_test() {
> > +   a="$1" b="$2"
> > +
> > +   printf "$a" >file
> > +   git add file
> > +   git commit -m"Add a file" >/dev/null
> 
> Avoid sticking a short-option to its argument, i.e.
> 
> git commit -m "Add a file"
> 
> > +
> > +   printf "$b" >file
> > +   git commit -am"Update a file" >/dev/null
> 
> Likewise.
> 
> git commit -a -m "Update a file"
> 
> The remainder of the file invites the same set of questions and
> comments you see for dh_diff_test() above, so I won't repeat them.
> 
> Thanks.

thanks for the feedback.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] config: add conditional include

[Jeff King]

On Sat, Aug 13, 2016 at 03:40:59PM +0700, Duy Nguyen wrote:

> Ping..

There was some discussion after v4. I think the open issues are:

  - the commit message is rather terse (it should describe motivation,
and can refer to the docs for the "how")

  - the syntax might be more clear as:

   [include-if "gitdir:..."]

or

   [include "gitdir-is:..."]

  - there is an open question of whether we would like to go this route,
maintaining backwards compatibility syntactically (and thus having
these includes quietly skipped in older versions), or introduce a
breaking syntax that could be more convenient, like:

  [if-gitdir(...):section]
  conditional-but-no-include-necessary = true

I do not have a strong opinion myself, though I had written the
original [include] assuming syntactic compatibility, and I think it
has worked out (e.g., you can manipulate include.path via "git
config" just as you would any other key).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: upload-pack/ls-remote: showing non-HEAD symbolic refs?

[Jeff King]

On Tue, Aug 16, 2016 at 03:24:14PM -0700, Josh Triplett wrote:

> > But if we had a real full-duplex connection over https, I think there
> > would be no reason for git:// to continue existing (we'd probably keep
> > ssh as it's a useful protocol for authentication, though).
> 
> Agreed.
> 
> Using ALPN wouldn't actually end up using HTTPS; it would negotiate with
> the server and end up connected directly to a git program speaking an
> arbitrary protocol over TLS.  Many web servers already support ALPN to
> negotiate HTTP/2, so this seems plausible.

It sounds like that would lose one of the nice properties of
git-over-http, which is that it works through arbitrary proxies (for
some people on restricted networks, they are stuck going through a
company-wide proxy, and neither git:// nor ssh are options in the first
place).

Maybe the world is heading in the direction of supporting ALPN
everywhere, but I suspect we'll be dealing with older proxies for a
while.

> Another alternative would be to define a framing for a full-duplex
> git-upload-pack connection inside a single HTTP/2 connection; HTTP/2
> already effectively allows full-duplex asynchronous conversations.

This has some of the same problem as above, though I'd bet on HTTP/2
support becoming mainstream more quickly.

I'm not very well educated on HTTP/2, but my understanding is that it
_doesn't_ just provide a full-duplex asynchronous connection out of the
box. You get "server push", but that is not quite the same thing.

So I'm not sure if we can make something work on top of those building
blocks, and if we do, how well it would fare with standard components
like proxies in the middle.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

[Jeff King]

On Fri, Aug 19, 2016 at 03:09:19PM +0200, Johannes Schindelin wrote:

> > The object name can have spaces in it, too. E.g.:
> > 
> >   HEAD:path with spaces
> > 
> > or even:
> > 
> >   :/grep for this
> > 
> > (as was pointed out to me when I tried to turn on %(rest) handling by
> > default, long ago). How do those work with your patch?
> 
> They don't ;-)
> 
> And quite frankly, the documentation should make it clear to users that
> batch mode with --filters or --textconv won't work when the object name
> contains white space: it says that the object name is split from the path
> at the first white space character.

I think that is an obvious implication of the new documentation. I was
just concerned with a regression from the previous behavior.

> > It looks like the extra split isn't enabled unless one of those options
> > is selected. Since --filters is new, that's OK for backwards
> > compatibility. But --textconv isn't.
> 
> Except that it is okay, because --textconv *was not even supported in
> batch mode*. So there is no backwards compatibility that could be broken.

Ah, OK. I thought we handled "HEAD:path with spaces" there, but I see
that you cannot even specify "--textconv" with "--batch", because it
complains of the cmdmode.

So the new rule becomes "we split if we see %(rest), or --textconv, or
--filter", which is reasonable.

> Fixing %(rest) for object names containing spaces is distinctly outside
> the original intent, and certainly outside of my use case.

Yeah, I agree it is not necessary for this series (I was only
considering it as an option to fix the regression I now see doesn't
exist).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

[Johannes Schindelin]

Hi Junio,

On Thu, 18 Aug 2016, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Thu, Aug 18, 2016 at 02:46:28PM +0200, Johannes Schindelin wrote:
> >
> >> With this patch, --batch can be combined with --textconv or --filters.
> >> For this to work, the input needs to have the form
> >> 
> >>
> >> 
> >> so that the filters can be chosen appropriately.
> >
> > The object name can have spaces in it, too. E.g.:
> >
> >   HEAD:path with spaces
> >
> > or even:
> >
> >   :/grep for this
> 
> When I wrote my review, I didn't consider this use case.
> 
> There is no -z format in --batch, which is unfortunate.  If we had
> one, it would trivially make it possible to do so, and we can even
> have paths with LF in them ;-).  On the other hand, producing a NUL
> separated input is a chore.

I think it would make for a fine little project to add support for --batch
-z.

Just not in this here patch series.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

[Johannes Schindelin]

Hi Peff,

On Thu, 18 Aug 2016, Jeff King wrote:

> On Thu, Aug 18, 2016 at 02:46:28PM +0200, Johannes Schindelin wrote:
> 
> > With this patch, --batch can be combined with --textconv or --filters.
> > For this to work, the input needs to have the form
> > 
> > 
> > 
> > so that the filters can be chosen appropriately.
> 
> The object name can have spaces in it, too. E.g.:
> 
>   HEAD:path with spaces
> 
> or even:
> 
>   :/grep for this
> 
> (as was pointed out to me when I tried to turn on %(rest) handling by
> default, long ago). How do those work with your patch?

They don't ;-)

And quite frankly, the documentation should make it clear to users that
batch mode with --filters or --textconv won't work when the object name
contains white space: it says that the object name is split from the path
at the first white space character.

> It looks like the extra split isn't enabled unless one of those options
> is selected. Since --filters is new, that's OK for backwards
> compatibility. But --textconv isn't.

Except that it is okay, because --textconv *was not even supported in
batch mode*. So there is no backwards compatibility that could be broken.

> I wonder if we need an option specifically to say "the object name can
> be split". Right now it kicks in automatically if you use "%(rest)" in
> your format, but you might not care about passing along that output
> (e.g., a lot of times I am piping "rev-list" straight to cat-file, and I
> have to use a separate "cut" to throw away the pathnames).

As I said elsewhere in this thread: if anybody encounters that problem,
they are welcome to fix it themselves.

My patch series purpose is to introduce --filters and make it compatible
with the batch mode. As I imitated --textconv, it was both easy and
correct to make it compatible with the batch mode, too. But that is the
extent of this here patch series.

Fixing %(rest) for object names containing spaces is distinctly outside
the original intent, and certainly outside of my use case.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

[Johannes Schindelin]

Hi Junio,

On Thu, 18 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > With this patch, --batch can be combined with --textconv or --filters.
> > For this to work, the input needs to have the form
> >
> > 
> >
> > so that the filters can be chosen appropriately.
> >  --batch::
> >  --batch=::
> > Print object information and contents for each object provided
> > -   on stdin.  May not be combined with any other options or arguments.
> > -   See the section `BATCH OUTPUT` below for details.
> > +   on stdin.  May not be combined with any other options or arguments
> > +   except `--textconv` or `--filters`, in which case the input lines
> > +   also need to specify the path, separated by white space.  See the
> > +   section `BATCH OUTPUT` below for details.
> 
> Makes sense.  This format still allows
> 
>   HEAD: 
> 
> i.e. the object name is taken from path2 but we forget it and
> pretend that the blob sits at path2, which is a good feature.

Correct.

> If I am not mistaken, your cover letter alluded that it might be
> ideal to take "HEAD:" (nothing else) as input, grab ""
> and feed that to the filtering machinery, but you decided to stop
> short of doing that.  I actually think that it is the right thing to
> do for this feature to ignore the trailing : in the object
> name, iow, I agree with the result from the feature design POV.

I actually decided against it only because it would uglify the code. From
a user point of view, I am a bit annoyed having to say

:README README
:Makefile Makefile
:main.c main.c
[...]

> The only thing that somewhat is worrysome is what would happen to
> %(rest).

What would happen is that it would print out the path, as it is exactly
that "rest" field that is used in the implementation, piggybacking on that
very nice feature.

Of course, this means that you simply cannot send "SHA-1 path rest" to
cat-file --batch and expect that to work. Should anybody need that in the
future, they can give it a try themselves to patch cat-file.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johannes Schindelin]

Hi Junio,

On Thu, 18 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > As suggested by its name, the --filters option applies the filters
> > that are currently configured for the specified path.
> >
> > [...]
> 
> I think you can lose the last paragraph and enrich the first one
> instead.  The text as-given does not even say anything about the new
> option affecting only blobs.
> 
>   The --filters option applies the convert_to_working_tree()
>   filter for the path when showing the contents of a regular
>   file blob object.
> 
> or something like that.

Done.

Thanks,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johannes Schindelin]

Hi Torsten,

On Thu, 18 Aug 2016, Torsten Bögershausen wrote:

> On Thu, Aug 18, 2016 at 02:46:17PM +0200, Johannes Schindelin wrote:
> > As suggested by its name, the --filters option applies the filters
> 
> []
> > diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
>
> Does it make sense to integrate tests into t1006-cat-file ?

t1006 is already a 500+ line script, running 110 cases in 13 seconds over
here. I really rather have a new, small, concise and parallelizable
script.

> > +test_expect_success 'no filters with `git show`' '
> > +   git show HEAD:world.txt >actual &&
>
> I would prefer to have something using
>   cat >expect <<-\EOF &&
>   xxx
>   test_cmp expect actual
> to make it easier to debug in case of a failure ?

Actually, I find it much harder to debug these "the output must match
these precise bytes, else we fail" type of test cases. Instead, I describe
in a natural way what is expected:

! has_cr actual

Now, when the test fails, whoever is that poor soul tasked with debugging
and fixing the breakage knows *what* goes wrong, conceptually.

So I do not think it would be a good idea to change the test in the way
you requested.

And now I have a request of my own. When you quote parts of my mail, you
insert your answers without surrounding them by empty lines. That makes it
extraordinarily hard for me to read your mails (in fact, I almost missed
your second comment).

Could you please start surrounding your replies by empty lines?

Thank you,
Dscho

Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

[Johannes Schindelin]

Hi Torsten,

On Thu, 18 Aug 2016, Torsten Bögershausen wrote:

> On Thu, Aug 18, 2016 at 02:46:28PM +0200, Johannes Schindelin wrote:
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index 5ff58b3..5f91cf4 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -17,6 +17,7 @@ struct batch_options {
> > int print_contents;
> > int buffer_output;
> > int all_objects;
> > +   int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
> How do I read 'w' and 'c' ?
> wilter and cextconv ? Does it make sense to use an enum here ?
> Or a #define ?

Sorry, Torsten, this is not my doing. So I cannot explain why it is not an
enum.

I *guess* the rationale for 'c' being the cmdmode of --textconv is "c for
convert". That is why I chose "w as in worktree" as new cmdmode.

Ciao,
Dscho

[PATCH v1 0/1] Rename NotNormalized (NNO) into CRLF in index

[tboegi]

From: Torsten Bögershausen 

Here comes the promised cleanup of t0027:
- The wording NNO is removed and replaced by CRI
- No code changes
- Needs to go on top of next or pu or tb/t0027-raciness-fix
Torsten Bögershausen (1):
  t0027: Rename NotNormalized (NNO) into CRLF in index

 t/t0027-auto-crlf.sh | 122 +--
 1 file changed, 61 insertions(+), 61 deletions(-)

-- 
2.9.3.599.g2376d31.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 1/1] t0027: Rename NotNormalized (NNO) into CRLF in index

[tboegi]

From: Torsten Bögershausen 

Originally NNO stands for content, that had been commited
"Not NOrmalized", in other words files with CRLF in the index.

Make more clear what should be tested:
- commit a file with CRLF into the index
- Change the content in the working tree
- Run "git add" and check for the conversion warnings
- Repeat for different content (text, LF, CRLF, mixed) and
  binary (LF and lone CR, CRLF with NUL)

Rename commit_chk_wrnNNO() into CRI_add_chk_wrn() and rename NNO into CRI.

Integrate create_NNO_files() into 'setup master'

Signed-off-by: Torsten Bögershausen 
---
 t/t0027-auto-crlf.sh | 122 +--
 1 file changed, 61 insertions(+), 61 deletions(-)

diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 90db54c..bfcf14b 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -49,24 +49,6 @@ create_gitattributes () {
} >.gitattributes
 }
 
-create_NNO_files () {
-   for crlf in false true input
-   do
-   for attr in "" auto text -text
-   do
-   for aeol in "" lf crlf
-   do
-   pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
-   cp CRLF_mix_LF ${pfx}_LF.txt &&
-   cp CRLF_mix_LF ${pfx}_CRLF.txt &&
-   cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&
-   cp CRLF_mix_LF ${pfx}_LF_mix_CR.txt &&
-   cp CRLF_mix_LF ${pfx}_CRLF_nul.txt
-   done
-   done
-   done
-}
-
 check_warning () {
case "$1" in
LF_CRLF) echo "warning: LF will be replaced by CRLF" >"$2".expect ;;
@@ -102,7 +84,7 @@ commit_check_warn () {
check_warning "$crlfnul" ${pfx}_CRLF_nul.err
 }
 
-commit_chk_wrnNNO () {
+CRI_add_chk_wrn () {
attr=$1 ; shift
aeol=$1 ; shift
crlf=$1 ; shift
@@ -111,7 +93,7 @@ commit_chk_wrnNNO () {
lfmixcrlf=$1 ; shift
lfmixcr=$1 ; shift
crlfnul=$1 ; shift
-   pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
+   pfx=CRI_attr_${attr}_aeol_${aeol}_${crlf}
#Commit files on top of existing file
create_gitattributes "$attr" $aeol &&
for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul
@@ -122,22 +104,22 @@ commit_chk_wrnNNO () {
git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
done
 
-   test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
+   test_expect_success "CRLF in index add file crlf=$crlf attr=$attr LF" '
check_warning "$lfwarn" ${pfx}_LF.err
'
-   test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf 
CRLF" '
+   test_expect_success "CRLF in index add file attr=$attr aeol=$aeol 
crlf=$crlf CRLF" '
check_warning "$crlfwarn" ${pfx}_CRLF.err
'
 
-   test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf 
CRLF_mix_LF" '
+   test_expect_success "CRLF in index add file attr=$attr aeol=$aeol 
crlf=$crlf CRLF_mix_LF" '
check_warning "$lfmixcrlf" ${pfx}_CRLF_mix_LF.err
'
 
-   test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf 
LF_mix_cr" '
+   test_expect_success "CRLF in index add file attr=$attr aeol=$aeol 
crlf=$crlf LF_mix_cr" '
check_warning "$lfmixcr" ${pfx}_LF_mix_CR.err
'
 
-   test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf 
CRLF_nul" '
+   test_expect_success "CRLF in index add file attr=$attr aeol=$aeol 
crlf=$crlf CRLF_nul" '
check_warning "$crlfnul" ${pfx}_CRLF_nul.err
'
 }
@@ -199,7 +181,7 @@ check_files_in_repo () {
compare_files $crlfnul ${pfx}CRLF_nul.txt
 }
 
-check_in_repo_NNO () {
+check_in_repo_CRI () {
attr=$1 ; shift
aeol=$1 ; shift
crlf=$1 ; shift
@@ -208,7 +190,7 @@ check_in_repo_NNO () {
lfmixcrlf=$1 ; shift
lfmixcr=$1 ; shift
crlfnul=$1 ; shift
-   pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
+   pfx=CRI_attr_${attr}_aeol_${aeol}_${crlf}
test_expect_success "compare_files $lfname ${pfx}_LF.txt" '
compare_files $lfname ${pfx}_LF.txt
'
@@ -329,8 +311,22 @@ test_expect_success 'setup master' '
printf "\$Id:  
\$\r\nLINEONE\r\nLINETWO\rLINETHREE"   >CRLF_mix_CR &&
printf "\$Id:  
\$\r\nLINEONEQ\r\nLINETWO\r\nLINETHREE" | q_to_nul >CRLF_nul &&
printf "\$Id:  
\$\nLINEONEQ\nLINETWO\nLINETHREE" | q_to_nul >LF_nul &&
-   create_NNO_files CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF 
CRLF_mix_LF &&
-   git -c core.autocrlf=false add NNO_*.txt &&
+   for crlf in false true input
+   do
+   for attr 

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

[Torsten Bögershausen]

[]
On Thu, Aug 18, 2016 at 02:46:17PM +0200, Johannes Schindelin wrote:

> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
> index 071029b..7d48735 100644
> --- a/Documentation/git-cat-file.txt
> +++ b/Documentation/git-cat-file.txt
> @@ -9,15 +9,15 @@ git-cat-file - Provide content or type and size information 
> for repository objec
>  SYNOPSIS
>  
>  [verse]
> -'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | 
> -p |  | --textconv ) 
> +'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | 
> -p |  | --textconv | --filters ) 
>  'git cat-file' (--batch | --batch-check) [--follow-symlinks]
>  
>  DESCRIPTION
>  ---
>  In its first form, the command provides the content or the type of an object 
> in
>  the repository. The type is required unless `-t` or `-p` is used to find the
> -object type, or `-s` is used to find the object size, or `--textconv` is used
> -(which implies type "blob").
> +object type, or `-s` is used to find the object size, or `--textconv` or
> +`--filters` is used (which imply type "blob").
>  
>  In the second form, a list of objects (separated by linefeeds) is provided on
>  stdin, and the SHA-1, type, and size of each object is printed on stdout.
> @@ -58,6 +58,12 @@ OPTIONS
>   order to apply the filter to the content recorded in the index at
>   .
>  
> +--filters::
> + Show the content as transformed by the filters configured in
Minor comment:
s/transformed/converted/ ?

Does it make sense to be more specific here:
The order of conversion is
- ident
- CRLF
- smudge
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Revert "display HTML in default browser using Windows' shell API"

[Johannes Schindelin]

Since 4804aab (help (Windows): Display HTML in default browser using
Windows' shell API, 2008-07-13), Git for Windows used to call
`ShellExecute()` to launch the default Windows handler for `.html`
files.

The idea was to avoid going through a shell script, for performance
reasons.

However, this change ignores the `help.browser` config setting. Together
with browsing help not being a performance-critical operation, let's
just revert that patch.

Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/mingw-help-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-help-v1

 builtin/help.c |  7 ---
 compat/mingw.c | 42 --
 compat/mingw.h |  3 ---
 3 files changed, 52 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 8848013..e8f79d7 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -379,17 +379,10 @@ static void get_html_page_path(struct strbuf *page_path, 
const char *page)
free(to_free);
 }
 
-/*
- * If open_html is not defined in a platform-specific way (see for
- * example compat/mingw.h), we use the script web--browse to display
- * HTML.
- */
-#ifndef open_html
 static void open_html(const char *path)
 {
execl_git_cmd("web--browse", "-c", "help.browser", path, (char *)NULL);
 }
-#endif
 
 static void show_html_page(const char *git_cmd)
 {
diff --git a/compat/mingw.c b/compat/mingw.c
index 2b5467d..3fbfda5 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1930,48 +1930,6 @@ int mingw_raise(int sig)
}
 }
 
-
-static const char *make_backslash_path(const char *path)
-{
-   static char buf[PATH_MAX + 1];
-   char *c;
-
-   if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
-   die("Too long path: %.*s", 60, path);
-
-   for (c = buf; *c; c++) {
-   if (*c == '/')
-   *c = '\\';
-   }
-   return buf;
-}
-
-void mingw_open_html(const char *unixpath)
-{
-   const char *htmlpath = make_backslash_path(unixpath);
-   typedef HINSTANCE (WINAPI *T)(HWND, const char *,
-   const char *, const char *, const char *, INT);
-   T ShellExecute;
-   HMODULE shell32;
-   int r;
-
-   shell32 = LoadLibrary("shell32.dll");
-   if (!shell32)
-   die("cannot load shell32.dll");
-   ShellExecute = (T)GetProcAddress(shell32, "ShellExecuteA");
-   if (!ShellExecute)
-   die("cannot run browser");
-
-   printf("Launching default browser to display HTML ...\n");
-   r = HCAST(int, ShellExecute(NULL, "open", htmlpath,
-   NULL, "\\", SW_SHOWNORMAL));
-   FreeLibrary(shell32);
-   /* see the MSDN documentation referring to the result codes here */
-   if (r <= 32) {
-   die("failed to launch browser for %.*s", MAX_PATH, unixpath);
-   }
-}
-
 int link(const char *oldpath, const char *newpath)
 {
typedef BOOL (WINAPI *T)(LPCWSTR, LPCWSTR, LPSECURITY_ATTRIBUTES);
diff --git a/compat/mingw.h b/compat/mingw.h
index 95e128f..2cadb81 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -417,9 +417,6 @@ int mingw_offset_1st_component(const char *path);
 #include 
 #endif
 
-void mingw_open_html(const char *path);
-#define open_html mingw_open_html
-
 /**
  * Converts UTF-8 encoded string to UTF-16LE.
  *
-- 
2.9.2.691.g78954f3

base-commit: d63263a4dee8fc7da9b97bbdedf9c0d1f33024d4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] help: introduce option --command-only

[Johannes Schindelin]

Hi Ralf,

On Thu, 18 Aug 2016, Ralf Thielow wrote:

> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> new file mode 100755
> index 000..e20f907
> --- /dev/null
> +++ b/t/t0012-help.sh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +
> +test_description='help'
> +
> +. ./test-lib.sh
> +
> +test_expect_success "works for commands and guides by default" "
> + git help status &&
> + git help revisions
> +"

Apart from using double quotes (which is inconsistent with the single
quotes used literally everwhere else in the test suite), this test is
incorrect. If the man page is not *installed*, it will fail:

$ sudo mv /usr/share/man/man1/git-status.1.gz \
/usr/share/man/man1/git-status.old.1.gz

$ sh t0012-help.sh -i -v -x
Initialized empty Git repository in .../trash directory.t0012-help/.git/
expecting success:
git help status &&
git help revisions

+ git help status
No manual entry for git-status
See 'man 7 undocumented' for help when manual pages are not available.
error: last command exited with $?=16
not ok 1 - works for commands and guides by default
#
#   git help status &&
#   git help revisions
#

It gets even worse.

On Windows, the default format is *not* man pages but html pages. So those
would have to be installed, too, to guarantee that the test succeeds.

It gets *even* worse.

On Windows, there is really no central location for man/html pages for
documentation, so we have to emulate that "prefix" (which is typically
/usr on Linux) via a "runtime prefix", i.e. a prefix determined relative
to the location of the currently running git executable. In the test
suite's case, it is typically the top-level directory of the git.git
checkout [*1*]. There are no man/html pages in that directory structure by
default (I, for one, rarely build them myself), and certainly not in the
place expected by this test.

It gets *even worse*.

Since the help.format is html on Windows, the page is opened by the
default viewer for HTML pages. So even if all of the above would be fixed,
running t0012-help of a supposedly unsupervised test suite would open new
tabs in the web browser. Probably forcing it into the foreground, too.

So how about fixing that? I would suggest to do it this way:

- configure help.format = html (for "man", the current code would always
  add $(prefix)/share/man to the MANPATH when testing, not what we want,
  and hacking this code *just* for testing is both ugly and unnecessary).

- configure help.htmlpath to point to a subdirectory that is created and
  populated in the same test script.

- configure help.browser to point to a script that is created in the same
  script and whose output we can verify, too.

The last point actually requires a patch that was recently introduced into
Git for Windows [*1*] (and that did not make it upstream yet) which
reverts that change whereby web--browse was sidestepped. That sidestepping
was well-intentioned but turned out to cause more harm than good.

Ciao,
Johannes

Footnote *1*: That statement is actually not even correct. As the git
executable can live in both $(prefix)/bin/ and $(prefix)/libexec/git-core,
i.e. at different directory levels below the prefix, we need to inspect
the *name* of the directory in which git.exe lives, and a git.git checkout
typically lives in a .../git/ directory which matches *none* of the
expected suffixes, so the runtime prefix defaults to "/", i.e. the
*current drive's root directory*. So your current test would only succeed
if the man pages for git-status and gitrevisions were copied into
C:\mingw64\share\man\man1!

Footnote *2*: https://github.com/git-for-windows/git/commit/243c72f5b0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html