Re: What's cooking in git.git (Dec 2017, #02; Thu, 7)

[Torsten Bögershausen]

> * tb/check-crlf-for-safe-crlf (2017-11-27) 1 commit
>   (merged to 'next' on 2017-12-05 at 7adaa1fe01)
>  + convert: tighten the safe autocrlf handling
> 
>  The "safe crlf" check incorrectly triggered for contents that does
>  not use CRLF as line endings, which has been corrected.
> 
>  Broken on Windows???
>  cf. 

Yes, broken on Windows. A fix is coming the next days.

[PATCH] docs/pretty-formats: mention commas in %(trailers) syntax

[Jeff King]

Commit 84ff053d47 (pretty.c: delimit "%(trailers)" arguments
with ",", 2017-10-01) switched the syntax of the trailers
placeholder, but forgot to update the documentation in
pretty-formats.txt.

There's need to mention the old syntax; it was never in a
released version of Git.

Signed-off-by: Jeff King 
---
This should go on top of tb/delimit-pretty-trailers-args-with-comma.

 Documentation/pretty-formats.txt | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index d433d50f81..e664c088a5 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -204,11 +204,13 @@ endif::git-rev-list[]
   than given and there are spaces on its left, use those spaces
 - '%><()', '%><|()': similar to '% <()', '%<|()'
   respectively, but padding both sides (i.e. the text is centered)
-- %(trailers): display the trailers of the body as interpreted by
-  linkgit:git-interpret-trailers[1]. If the `:only` option is given,
-  omit non-trailer lines from the trailer block.  If the `:unfold`
-  option is given, behave as if interpret-trailer's `--unfold` option
-  was given. E.g., `%(trailers:only:unfold)` to do both.
+- %(trailers[:options]): display the trailers of the body as interpreted
+  by linkgit:git-interpret-trailers[1]. The `trailers` string may be
+  followed by a colon and zero or more comma-separated options. If the
+  `only` option is given, omit non-trailer lines from the trailer block.
+  If the `unfold` option is given, behave as if interpret-trailer's
+  `--unfold` option was given.  E.g., `%(trailers:only,unfold)` to do
+  both.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
-- 
2.15.1.659.g8bd2eae3ea

[PATCH 0/1] diffcore-blobfind

[Stefan Beller]

This includes the suggestions by Junio,

Thanks,
Stefan

interdiff to currently queued below.

Stefan Beller (1):
  diffcore: add a filter to find a specific blob

 Documentation/diff-options.txt |  5 +
 Makefile   |  1 +
 builtin/log.c  |  2 +-
 diff.c | 20 +++-
 diff.h |  3 +++
 diffcore-blobfind.c| 41 +
 diffcore.h |  1 +
 revision.c |  5 -
 t/t4064-diff-blobfind.sh   | 35 +++
 9 files changed, 110 insertions(+), 3 deletions(-)
 create mode 100644 diffcore-blobfind.c
 create mode 100755 t/t4064-diff-blobfind.sh

-- 
2.15.1.424.g9478a66081-goog

diff --git c/Documentation/diff-options.txt w/Documentation/diff-options.txt
index 252a21cc19..34d53b95f1 100644
--- c/Documentation/diff-options.txt
+++ w/Documentation/diff-options.txt
@@ -500,6 +500,7 @@ information.
 --pickaxe-regex::
Treat the  given to `-S` as an extended POSIX regular
expression to match.
+
 --blobfind=::
Restrict the output such that one side of the diff
matches the given blob-id.
diff --git c/diffcore-blobfind.c w/diffcore-blobfind.c
index 5d222fc336..e65c7cad6e 100644
--- c/diffcore-blobfind.c
+++ w/diffcore-blobfind.c
@@ -8,40 +8,30 @@
 static void diffcore_filter_blobs(struct diff_queue_struct *q,
  struct diff_options *options)
 {
-   int i, j = 0, c = q->nr;
+   int src, dst;
 
if (!options->blobfind)
BUG("blobfind oidset not initialized???");
 
-   for (i = 0; i < q->nr; i++) {
-   struct diff_filepair *p = q->queue[i];
+   for (src = dst = 0; src < q->nr; src++) {
+   struct diff_filepair *p = q->queue[src];
 
-   if (DIFF_PAIR_UNMERGED(p) ||
-   (DIFF_FILE_VALID(p->one) &&
+   if ((DIFF_FILE_VALID(p->one) &&
 oidset_contains(options->blobfind, >one->oid)) ||
(DIFF_FILE_VALID(p->two) &&
-oidset_contains(options->blobfind, >two->oid)))
-   continue;
-
-   diff_free_filepair(p);
-   q->queue[i] = NULL;
-   c--;
-   }
-
-   /* Keep it sorted. */
-   i = 0; j = 0;
-   while (i < c) {
-   while (!q->queue[j])
-   j++;
-   q->queue[i] = q->queue[j];
-   i++; j++;
+oidset_contains(options->blobfind, >two->oid))) {
+   q->queue[dst] = p;
+   dst++;
+   } else {
+   diff_free_filepair(p);
+   }
}
 
-   q->nr = c;
-
-   if (!c) {
+   if (!dst) {
free(q->queue);
DIFF_QUEUE_CLEAR(q);
+   } else {
+   q->nr = dst;
}
 }
 
diff --git c/revision.c w/revision.c
index 6449619c0a..8e1a89f832 100644
--- c/revision.c
+++ w/revision.c
@@ -2884,6 +2884,8 @@ int prepare_revision_walk(struct rev_info *revs)
simplify_merges(revs);
if (revs->children.name)
set_children(revs);
+   if (revs->diffopt.blobfind)
+   revs->simplify_history = 0;
return 0;
 }
 

[PATCH 1/1] diffcore: add a filter to find a specific blob

[Stefan Beller]

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

One might be tempted to extend git-describe to also work with blobs,
such that `git describe ` gives a description as
':'.  This was implemented at [2]; as seen by the sheer
number of responses (>110), it turns out this is tricky to get right.
The hard part to get right is picking the correct 'commit-ish' as that
could be the commit that (re-)introduced the blob or the blob that
removed the blob; the blob could exist in different branches.

Junio hinted at a different approach of solving this problem, which this
patch implements. Teach the diff machinery another flag for restricting
the information to what is shown. For example:

  $ ./git log --oneline --blobfind=v2.0.0:Makefile
  b2feb64309 Revert the whole "ask curl-config" topic for now
  47fbfded53 i18n: only extract comments marked with "TRANSLATORS:"

we observe that the Makefile as shipped with 2.0 was introduced in
v1.9.2-471-g47fbfded53 and replaced in v2.0.0-rc1-5-gb2feb64309 by
a different blob.

[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
[2] https://public-inbox.org/git/20171028004419.10139-1-sbel...@google.com/

Signed-off-by: Stefan Beller 
---
 Documentation/diff-options.txt |  5 +
 Makefile   |  1 +
 builtin/log.c  |  2 +-
 diff.c | 20 +++-
 diff.h |  3 +++
 diffcore-blobfind.c| 41 +
 diffcore.h |  1 +
 revision.c |  5 -
 t/t4064-diff-blobfind.sh   | 35 +++
 9 files changed, 110 insertions(+), 3 deletions(-)
 create mode 100644 diffcore-blobfind.c
 create mode 100755 t/t4064-diff-blobfind.sh

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index dd0dba5b1d..34d53b95f1 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -500,6 +500,11 @@ information.
 --pickaxe-regex::
Treat the  given to `-S` as an extended POSIX regular
expression to match.
+
+--blobfind=::
+   Restrict the output such that one side of the diff
+   matches the given blob-id.
+
 endif::git-format-patch[]
 
 -O::
diff --git a/Makefile b/Makefile
index ee9d5eb11e..fdfa8f38f6 100644
--- a/Makefile
+++ b/Makefile
@@ -775,6 +775,7 @@ LIB_OBJS += date.o
 LIB_OBJS += decorate.o
 LIB_OBJS += diffcore-break.o
 LIB_OBJS += diffcore-delta.o
+LIB_OBJS += diffcore-blobfind.o
 LIB_OBJS += diffcore-order.o
 LIB_OBJS += diffcore-pickaxe.o
 LIB_OBJS += diffcore-rename.o
diff --git a/builtin/log.c b/builtin/log.c
index 6c1fa896ad..7b91f61423 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -181,7 +181,7 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
init_display_notes(>notes_opt);
 
if (rev->diffopt.pickaxe || rev->diffopt.filter ||
-   rev->diffopt.flags.follow_renames)
+   rev->diffopt.flags.follow_renames || rev->diffopt.blobfind)
rev->always_show_header = 0;
 
if (source)
diff --git a/diff.c b/diff.c
index 0763e89263..8861f89ab1 100644
--- a/diff.c
+++ b/diff.c
@@ -4082,6 +4082,7 @@ void diff_setup(struct diff_options *options)
options->interhunkcontext = diff_interhunk_context_default;
options->ws_error_highlight = ws_error_highlight_default;
options->flags.rename_empty = 1;
+   options->blobfind = NULL;
 
/* pathchange left =NULL by default */
options->change = diff_change;
@@ -4487,6 +4488,19 @@ static int parse_ws_error_highlight_opt(struct 
diff_options *opt, const char *ar
return 1;
 }
 
+static int parse_blobfind_opt(struct diff_options *opt, const char *arg)
+{
+   struct object_id oid;
+
+   if (get_oid_blob(arg, ) || sha1_object_info(oid.hash, NULL) != 
OBJ_BLOB)
+   return error("object '%s' is not a blob", arg);
+
+   if (!opt->blobfind)
+   opt->blobfind = xcalloc(1, sizeof(*opt->blobfind));
+   oidset_insert(opt->blobfind, );
+   return 1;
+}
+
 int diff_opt_parse(struct diff_options *options,
   const char **av, int ac, const char *prefix)
 {
@@ -4736,7 +4750,8 @@ int diff_opt_parse(struct diff_options *options,
else if ((argcount = short_opt('O', av, ))) {
options->orderfile = prefix_filename(prefix, optarg);
return argcount;
-   }
+   } else if (skip_prefix(arg, "--blobfind=", ))
+   return parse_blobfind_opt(options, arg);
else if ((argcount = parse_long_opt("diff-filter", av, ))) {
int offending = parse_diff_filter_opt(optarg, options);
if (offending)
@@ -5770,6 +5785,9 @@ void diffcore_std(struct diff_options *options)

Re: [SCRIPT/RFC 0/3] git-commit --onto-parent (three-way merge, no working tree file changes)

[Igor Djordjevic]

On 06/12/2017 19:34, Johannes Sixt wrote:
> 
> I am sorry for not responding in detail. I think we've reached a 
> mutual understanding of our workflows.

No problem, thanks for your time so far.

There might be one more thing I should address, possibly left unclear 
from my previous message, but I`ll leave that for a follow-up e-mail, 
not being that important at the moment for the topic itself.

On 06/12/2017 19:40, Junio C Hamano wrote:
> 
> > Though, from the ideas you tossed around most recently, you seem to
> > want to make git-commit into a kitchen-sink for everything. I have
> > my doubts that this will be a welcome change. Just because new
> > commits are created does not mean that the feature must live in
> > git-commit.
> 
> Nicely put.

Yeah, I understand that might have felt cluttering, besides also 
being out of scope of the original topic idea. Thanks for the reality 
check (to both).

To get back on track, and regarding what`s already been said, would 
having something like this(1) feel useful?

(1) git commit --onto 

So in previously mentioned situation:

(2) ...A...C<- topics A, C
\   \
  ---o---o---o---o I<- integration <- HEAD
/   /
...B...D<- topics B, D

... it would allow committing changes F inside HEAD on top of B 
directly, no checkout / branch switching needed, getting to:

(3) ...A...C<- topics A, C
\   \
  ---o---o---o---o I<- integration <- HEAD
/   /
...B...D<- topic D
\
 F  <- topic B

So the most conservative approach, where changes F are removed from 
HEAD index and working tree, leaving it up to the user to decide if 
he will then merge them back in (or do something else).

I stress the major selling point here still being avoiding branch 
switching back and forth in order to commit a fixup on a different 
branch, which could otherwise trigger needless rebuilds, being 
significant in large projects.

And thanks to that `git-merge-one-file--cached`[1] script, we are 
also able to resolve some more of trivial conflicts when applying F 
onto B, using three-way file merge when needed, but still not 
touching working tree (contrary to original `git-merge-one-file`).

Regards, Buga

[1] 
https://public-inbox.org/git/CAPc5daWupO6DMOMFGn=xjucg-jmyc4eyo8+tmasdwcaohxz...@mail.gmail.com/T/#mcb3953542dc265516e3ab1bff006ff1b5b85126a

[PATCH] decorate: clean up and document API

[Jonathan Tan]

Improve the names of the identifiers in decorate.h, document them, and
add an example of how to use these functions.

The example is compiled and run as part of the test suite.

Signed-off-by: Jonathan Tan 
---
This patch contains some example code in a test helper.

There is a discussion at [1] about where example code for hashmap should
go. For something relatively simple, like decorate, perhaps example code
isn't necessary; but if we include example code anyway, I think that we
might as well make it compiled and tested, so this patch contains that
example code in a test helper.

[1] 
https://public-inbox.org/git/466dd5331907754ca5c25cc83173ca895220ca81.1511999045.git.johannes.schinde...@gmx.de/
---
 Documentation/technical/api-decorate.txt |  6 ---
 Makefile |  1 +
 builtin/fast-export.c|  2 +-
 decorate.c   | 28 ++--
 decorate.h   | 49 +++--
 t/helper/.gitignore  |  1 +
 t/helper/test-example-decorate.c | 74 
 t/t9004-example.sh   | 10 +
 8 files changed, 146 insertions(+), 25 deletions(-)
 delete mode 100644 Documentation/technical/api-decorate.txt
 create mode 100644 t/helper/test-example-decorate.c
 create mode 100755 t/t9004-example.sh

diff --git a/Documentation/technical/api-decorate.txt 
b/Documentation/technical/api-decorate.txt
deleted file mode 100644
index 1d52a6ce1..0
--- a/Documentation/technical/api-decorate.txt
+++ /dev/null
@@ -1,6 +0,0 @@
-decorate API
-
-
-Talk about 
-
-(Linus)
diff --git a/Makefile b/Makefile
index fef9c8d27..df52cc1c3 100644
--- a/Makefile
+++ b/Makefile
@@ -651,6 +651,7 @@ TEST_PROGRAMS_NEED_X += test-dump-cache-tree
 TEST_PROGRAMS_NEED_X += test-dump-fsmonitor
 TEST_PROGRAMS_NEED_X += test-dump-split-index
 TEST_PROGRAMS_NEED_X += test-dump-untracked-cache
+TEST_PROGRAMS_NEED_X += test-example-decorate
 TEST_PROGRAMS_NEED_X += test-fake-ssh
 TEST_PROGRAMS_NEED_X += test-genrandom
 TEST_PROGRAMS_NEED_X += test-hashmap
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index f8fe04ca5..796d0cd66 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -895,7 +895,7 @@ static void export_marks(char *file)
 {
unsigned int i;
uint32_t mark;
-   struct object_decoration *deco = idnums.hash;
+   struct decoration_entry *deco = idnums.entries;
FILE *f;
int e = 0;
 
diff --git a/decorate.c b/decorate.c
index 270eb2519..de31331fa 100644
--- a/decorate.c
+++ b/decorate.c
@@ -14,20 +14,20 @@ static unsigned int hash_obj(const struct object *obj, 
unsigned int n)
 static void *insert_decoration(struct decoration *n, const struct object 
*base, void *decoration)
 {
int size = n->size;
-   struct object_decoration *hash = n->hash;
+   struct decoration_entry *entries = n->entries;
unsigned int j = hash_obj(base, size);
 
-   while (hash[j].base) {
-   if (hash[j].base == base) {
-   void *old = hash[j].decoration;
-   hash[j].decoration = decoration;
+   while (entries[j].base) {
+   if (entries[j].base == base) {
+   void *old = entries[j].decoration;
+   entries[j].decoration = decoration;
return old;
}
if (++j >= size)
j = 0;
}
-   hash[j].base = base;
-   hash[j].decoration = decoration;
+   entries[j].base = base;
+   entries[j].decoration = decoration;
n->nr++;
return NULL;
 }
@@ -36,24 +36,23 @@ static void grow_decoration(struct decoration *n)
 {
int i;
int old_size = n->size;
-   struct object_decoration *old_hash = n->hash;
+   struct decoration_entry *old_entries = n->entries;
 
n->size = (old_size + 1000) * 3 / 2;
-   n->hash = xcalloc(n->size, sizeof(struct object_decoration));
+   n->entries = xcalloc(n->size, sizeof(struct decoration_entry));
n->nr = 0;
 
for (i = 0; i < old_size; i++) {
-   const struct object *base = old_hash[i].base;
-   void *decoration = old_hash[i].decoration;
+   const struct object *base = old_entries[i].base;
+   void *decoration = old_entries[i].decoration;
 
if (!decoration)
continue;
insert_decoration(n, base, decoration);
}
-   free(old_hash);
+   free(old_entries);
 }
 
-/* Add a decoration pointer, return any old one */
 void *add_decoration(struct decoration *n, const struct object *obj,
void *decoration)
 {
@@ -64,7 +63,6 @@ void *add_decoration(struct decoration *n, const struct 
object *obj,
return insert_decoration(n, obj, decoration);
 }
 
-/* Lookup a decoration 

Re: [WIP 11/15] serve: introduce git-serve

[Junio C Hamano]

Brandon Williams  writes:

> +static struct protocol_capability *get_capability(const char *key)
> +{
> + int i;
> +
> + if (!key)
> + return NULL;
> +
> + for (i = 0; i < ARRAY_SIZE(capabilities); i++) {
> + struct protocol_capability *c = [i];
> + const char *out;
> + if (skip_prefix(key, c->name, ) && (!*out || *out == '='))
> + return c;

Looks familiar and resembles what was recently discussed on list ;-)

> +int cmd_serve(int argc, const char **argv, const char *prefix)
> +{
> +
> + struct option options[] = {
> + OPT_END()
> + };
> +
> + /* ignore all unknown cmdline switches for now */
> + argc = parse_options(argc, argv, prefix, options, grep_usage,
> +  PARSE_OPT_KEEP_DASHDASH |
> +  PARSE_OPT_KEEP_UNKNOWN);
> + serve();
> +
> + return 0;
> +}
> ...
> +/* Main serve loop for protocol version 2 */
> +void serve(void)
> +{
> + /* serve by default supports v2 */
> + packet_write_fmt(1, "version 2\n");
> +
> + advertise_capabilities();
> +
> + for (;;)
> + if (process_request())
> + break;
> +}

I am guessing that this would be run just like upload-pack,
i.e. invoked via ssh or via git-daemon, and that is why it can just
assume that fd#0/fd#1 are already connected to the other end.  It
may be helpful to document somewhere how we envision to invoke this
program.

Re: [PATCH v5 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[Junio C Hamano]

Junio C Hamano  writes:

> SZEDER Gábor  writes:
>
>> On Fri, Dec 1, 2017 at 6:59 AM, Kaartic Sivaraam
>>  wrote:
>>> Sorry, missed a ';' in v4.
>>>
>>> The surprising thing I discovered in the TravisCI build for v4
>>> was that apart from the 'Documentation' build the 'Static Analysis'
>>> build passed, with the following output,
>>>
>>> -- 
>>> $ ci/run-static-analysis.sh
>>> GIT_VERSION = 2.13.1.1972.g6ced3f745
>>>  SPATCH contrib/coccinelle/array.cocci
>>>  SPATCH result: contrib/coccinelle/array.cocci.patch
>>>  SPATCH contrib/coccinelle/free.cocci
>>>  SPATCH contrib/coccinelle/object_id.cocci
>>>  SPATCH contrib/coccinelle/qsort.cocci
>>>  SPATCH contrib/coccinelle/strbuf.cocci
>>>  SPATCH result: contrib/coccinelle/strbuf.cocci.patch
>>>  SPATCH contrib/coccinelle/swap.cocci
>>>  SPATCH contrib/coccinelle/xstrdup_or_null.cocci
>>>
>>> The command "ci/run-static-analysis.sh" exited with 0.
>>
>> Perhaps Coccinelle should have errored out, or perhaps its 0 exit code
>> means "I didn't find any code matching any of the semantic patches that
>> required transformation".
>>
>>> I guess static analysis tools make an assumption that the source
>>> code is syntactically valid for them to work correctly. So, I guess
>>> we should at least make sure the code 'compiles' before running
>>> the static analysis tool even though we don't build it completely.
>>> I'm not sure if it's a bad thing to run the static analysis on code
>>> that isn't syntactically valid, though.
>>
>> Travis CI already runs 6 build jobs compiling Git.  And that is in
>> addition to the one that you should have run yourself before even
>> thinking about submitting v4 ;)  That's plenty to catch errors like
>> these.  And if any of those builds fail because Git can't be built or
>> because of a test failure, then Coccinelle's success doesn't matter at
>> all, because the commit is toast anyway.
>
> Somehow this fell underneath my radar horizon.  I see v4 and v5 of
> 4/4 but do not seem to find 1-3/4.  Is this meant to be a standalone
> patch, or am I expected to already have 1-3 that we already are
> committed to take?

Ah, I am guessing that this would apply on top of 1-3/4 in the
thread with <20171118172648.17918-1-kaartic.sivar...@gmail.com>

The base of the series seems to predate 16169285 ("Merge branch
'jc/branch-name-sanity'", 2017-11-28), so let me see how it looks by
applying those three plus this one on top of 'master' before that
point.

Re: [PATCH v5 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[Junio C Hamano]

SZEDER Gábor  writes:

> On Fri, Dec 1, 2017 at 6:59 AM, Kaartic Sivaraam
>  wrote:
>> Sorry, missed a ';' in v4.
>>
>> The surprising thing I discovered in the TravisCI build for v4
>> was that apart from the 'Documentation' build the 'Static Analysis'
>> build passed, with the following output,
>>
>> -- 
>> $ ci/run-static-analysis.sh
>> GIT_VERSION = 2.13.1.1972.g6ced3f745
>>  SPATCH contrib/coccinelle/array.cocci
>>  SPATCH result: contrib/coccinelle/array.cocci.patch
>>  SPATCH contrib/coccinelle/free.cocci
>>  SPATCH contrib/coccinelle/object_id.cocci
>>  SPATCH contrib/coccinelle/qsort.cocci
>>  SPATCH contrib/coccinelle/strbuf.cocci
>>  SPATCH result: contrib/coccinelle/strbuf.cocci.patch
>>  SPATCH contrib/coccinelle/swap.cocci
>>  SPATCH contrib/coccinelle/xstrdup_or_null.cocci
>>
>> The command "ci/run-static-analysis.sh" exited with 0.
>
> Perhaps Coccinelle should have errored out, or perhaps its 0 exit code
> means "I didn't find any code matching any of the semantic patches that
> required transformation".
>
>> I guess static analysis tools make an assumption that the source
>> code is syntactically valid for them to work correctly. So, I guess
>> we should at least make sure the code 'compiles' before running
>> the static analysis tool even though we don't build it completely.
>> I'm not sure if it's a bad thing to run the static analysis on code
>> that isn't syntactically valid, though.
>
> Travis CI already runs 6 build jobs compiling Git.  And that is in
> addition to the one that you should have run yourself before even
> thinking about submitting v4 ;)  That's plenty to catch errors like
> these.  And if any of those builds fail because Git can't be built or
> because of a test failure, then Coccinelle's success doesn't matter at
> all, because the commit is toast anyway.

Somehow this fell underneath my radar horizon.  I see v4 and v5 of
4/4 but do not seem to find 1-3/4.  Is this meant to be a standalone
patch, or am I expected to already have 1-3 that we already are
committed to take?

Thanks.

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Junio C Hamano]

Jeff King  writes:

> On Thu, Dec 07, 2017 at 01:59:39PM -0800, Junio C Hamano wrote:
>
>> Jeff King  writes:
>> 
>> > On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:
>> >
>> >> Signed-off-by: Junio C Hamano 
>> >> ---
>> >
>> > It might be worth mentioning why this conversion is pulled out from the
>> > others (because its "default" case is "do not touch the pointer").
>> 
>> I am not sure what you mean by "pulled out from the others".  I did
>> not intend to keep these 3 additional patches permanently; rather, I
>> did them to help Christian's rerolling the series, and I do not think
>> this one should be separate from other ones that use the _default()
>> variant when that happens.
>
> Ah, I see. I had thought you meant these to be applied on top.

Ah, I do not particularly mind doing things incrementally, either.

If this goes on top as a standalone patch, then the reason why it is
separate from the other users of _default() is not because the way
it uses the null return is special, but because it was written by a
different author, I would think.  

The reason why _default() variant exists is because its callers want
to react differently to "--foo" from the way they react to "--foo="
(with an empty string as its value), and from that point of view,
this caller is not all that different from other ones, like the one
that parses --color-words Christian wrote in his initial round.

Re: [WIP 03/15] pkt-line: add delim packet support

[Stefan Beller]

On Mon, Dec 4, 2017 at 3:58 PM, Brandon Williams  wrote:
> One of the design goals of protocol-v2 is to improve the semantics of
> flush packets.  Currently in protocol-v1, flush packets are used both to
> indicate a break in a list of packet lines as well as an indication that
> one side has finished speaking.  This makes it particularly difficult
> to implement proxies as a proxy would need to completely understand git
> protocol instead of simply looking for a flush packet.
>
> To do this, introduce the special deliminator packet '0001'.  A delim
> packet can then be used as a deliminator between lists of packet lines
> while flush packets can be reserved to indicate the end of a response.
>
> Signed-off-by: Brandon Williams 

I presume the update for Documentation/technical/* comes at a later patch in the
series, clarifying the exact semantic difference between the packet types?

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Jeff King]

On Thu, Dec 07, 2017 at 01:59:39PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:
> >
> >> Signed-off-by: Junio C Hamano 
> >> ---
> >
> > It might be worth mentioning why this conversion is pulled out from the
> > others (because its "default" case is "do not touch the pointer").
> 
> I am not sure what you mean by "pulled out from the others".  I did
> not intend to keep these 3 additional patches permanently; rather, I
> did them to help Christian's rerolling the series, and I do not think
> this one should be separate from other ones that use the _default()
> variant when that happens.

Ah, I see. I had thought you meant these to be applied on top.

-Peff

Re: [PATCH] diff: add tests for --relative without optional prefix value

[Jacob Keller]

On Thu, Dec 7, 2017 at 1:50 PM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> On Thu, Dec 07, 2017 at 11:01:35AM -0800, Jacob Keller wrote:
>>
>>> From: Jacob Keller 
>>>
>>> We already have tests for --relative, but they currently only test when
>>> a prefix has been provided. This fails to test the case where --relative
>>> by itself should use the current directory as the prefix.
>>>
>>> Teach the check_$type functions to take a directory argument to indicate
>>> which subdirectory to run the git commands in. Add a new test which uses
>>> this to test --relative without a prefix value.
>>
>> This looks good to me (and I slightly prefer it over Junio's for the
>> simplicity).
>>
>> I agree on the ordering suggestion Junio made.
>
> I also prefer this one over the other one, provided if it is ported
> on top of the preliminary clean-up I did in the other one.
>
> Thanks.
>

Yea. I can do that if you want.

Thanks,
Jake

Re: [WIP 02/15] pkt-line: introduce struct packet_reader

[Stefan Beller]

On Mon, Dec 4, 2017 at 3:58 PM, Brandon Williams  wrote:
> Sometimes it is advantageous to be able to peek the next packet line
> without consuming it (e.g. to be able to determine the protocol version
> a server is speaking).  In order to do that introduce 'struct
> packet_reader' which is an abstraction around the normal packet reading
> logic.  This enables a caller to be able to peek a single line at a time
> using 'packet_reader_peek()' and having a caller consume a line by
> calling 'packet_reader_read()'.
>
> Signed-off-by: Brandon Williams 
> ---
>  pkt-line.c | 61 +
>  pkt-line.h | 20 
>  2 files changed, 81 insertions(+)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index ac619f05b..518109bbe 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -406,3 +406,64 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct 
> strbuf *sb_out)
> }
> return sb_out->len - orig_len;
>  }
> +
> +/* Packet Reader Functions */
> +void packet_reader_init(struct packet_reader *reader, int fd,
> +   char *src_buffer, size_t src_len)
> +{
> +   reader->fd = fd;
> +   reader->src_buffer = src_buffer;
> +   reader->src_len = src_len;
> +   reader->buffer = packet_buffer;
> +   reader->buffer_size = sizeof(packet_buffer);
> +   reader->options = PACKET_READ_CHOMP_NEWLINE | 
> PACKET_READ_GENTLE_ON_EOF;

I assume the future user of this packet reader will need exactly
these options coincidentally. ;)

I think it might be ok for now and later we can extend the reader if needed
to also take the flags as args. However given this set of args, this is a gentle
packet reader, as it corresponds to the _gently version of reading
packets AFAICT.

Unlike the pkt_read function this constructor of a packet reader doesn't need
the arguments for its buffer (packet_buffer and sizeof thereof), which
packet_read
unfortunately needs. We pass in packet_buffer all the time except in
builtin/receive-pack
for obtaining the gpg cert. I think that's ok here.

> +enum packet_read_status packet_reader_read(struct packet_reader *reader)
> +{
> +   if (reader->line_peeked) {
> +   reader->line_peeked = 0;
> +   return reader->status;
> +   }
> +
> +   reader->status = packet_read_with_status(reader->fd,
> +>src_buffer,
> +>src_len,
> +reader->buffer,
> +reader->buffer_size,
> +>pktlen,
> +reader->options);
> +
> +   switch (reader->status) {
> +   case PACKET_READ_ERROR:
> +   reader->pktlen = -1;

In case of error the read function might not
have assigned the pktlen as requested, so we assign
it to -1/NULL here. Though the caller ought to already know
that they handle bogus, as the state is surely the first thing
they'd inspect?

> +   reader->line = NULL;
> +   break;
> +   case PACKET_READ_NORMAL:
> +   reader->line = reader->buffer;
> +   break;
> +   case PACKET_READ_FLUSH:
> +   reader->pktlen = 0;
> +   reader->line = NULL;
> +   break;
> +   }

Oh, this gives an interesting interface for someone who is
just inspecting the len/line instead of the state, so it might be
worth keeping it this way.

Can we have an API documentation in the header file,
explaining what to expect in each field given the state
of the (read, peaked) packet?

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Junio C Hamano]

Jeff King  writes:

> On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:
>
>> Signed-off-by: Junio C Hamano 
>> ---
>
> It might be worth mentioning why this conversion is pulled out from the
> others (because its "default" case is "do not touch the pointer").

I am not sure what you mean by "pulled out from the others".  I did
not intend to keep these 3 additional patches permanently; rather, I
did them to help Christian's rerolling the series, and I do not think
this one should be separate from other ones that use the _default()
variant when that happens.

Re: [PATCH] diff: add tests for --relative without optional prefix value

[Junio C Hamano]

Jeff King  writes:

> On Thu, Dec 07, 2017 at 11:01:35AM -0800, Jacob Keller wrote:
>
>> From: Jacob Keller 
>> 
>> We already have tests for --relative, but they currently only test when
>> a prefix has been provided. This fails to test the case where --relative
>> by itself should use the current directory as the prefix.
>> 
>> Teach the check_$type functions to take a directory argument to indicate
>> which subdirectory to run the git commands in. Add a new test which uses
>> this to test --relative without a prefix value.
>
> This looks good to me (and I slightly prefer it over Junio's for the
> simplicity).
>
> I agree on the ordering suggestion Junio made.

I also prefer this one over the other one, provided if it is ported
on top of the preliminary clean-up I did in the other one.

Thanks.

Re: [PATCH] hashmap: adjust documentation to reflect reality

[Johannes Schindelin]

Hi,

On Mon, 4 Dec 2017, Stefan Beller wrote:

> On Sat, Dec 2, 2017 at 9:35 PM, Junio C Hamano  wrote:
> > Jeff King  writes:
> >
> >> My second suggestion (which I'm on the fence about) is: would it better
> >> to just say "see t/helper/test-hashmap.c for a representative example?"
> 
> I think that may be better in the long run, indeed.

But we moved that example already out of the technical API documentation
into the hashmap.h file! Too much moving for my taste.

> > I also had the same thought.  It is rather unwieldy to ask people to
> > lift code from comment text, and it is also hard to maintain such a
> > commented out code to make sure it is up to date.
> >
> >> I'm all for code examples in documentation, but this one is quite
> >> complex. The code in test-hashmap.c is not much more complex, and is at
> >> least guaranteed to compile and run.
> >
> > Yup.  Exactly.
> >
> >> It doesn't show off how to combine a flex-array with a hashmap as
> >> well, but I'm not sure how important that is. So I could go either
> >> way.
> 
> We could add that example to the test helper as then we have a good (tested)
> example for that case, too.

What we could *also* do, and what would probably make *even more* sense,
is to simplify the example drastically, to a point where testing it in
test-hashmap is pointless, and where a reader can gather *very* quickly
what it takes to use the hashmap routines.

In any case, I would really like to see my patch applied first, as it is a
separate concern from what you gentle people talk about: I simply want
that incorrect documentation fixed. The earlier, the better.

Ciao,
Dscho

Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()

[Junio C Hamano]

René Scharfe  writes:

> Use string_list_append_nodup() instead of string_list_append() to hand
> over ownership of a detached strbuf and thus avoid leaking its memory.
>
> Signed-off-by: Rene Scharfe 
> ---
>  builtin/fmt-merge-msg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
> index 22034f87e7..8e8a15ea4a 100644
> --- a/builtin/fmt-merge-msg.c
> +++ b/builtin/fmt-merge-msg.c
> @@ -377,7 +377,8 @@ static void shortlog(const char *name,
>   string_list_append(,
>  oid_to_hex(>object.oid));
>   else
> - string_list_append(, strbuf_detach(, NULL));
> + string_list_append_nodup(,
> +  strbuf_detach(, NULL));
>   }
>  
>   if (opts->credit_people)

What is leaked comes from strbuf, so the title is not a lie, but I
tend to think that this leak is caused by a somewhat strange
string_list API.  The subjects string-list is initialized as a "dup"
kind, but a caller that wants to avoid leaking can (and should) use
_nodup() call to add a string without duping.  It all feels a bit
too convoluted.

The patch looks good.

Re: [PATCH 1/1] diffcore: add a filter to find a specific blob

[Junio C Hamano]

Junio C Hamano  writes:

After saying "Will merge to 'next'" in the recent "What's cooking"
report, I noticed that a few loose ends were never tied on this
topic.

>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
>> index dd0dba5b1d..252a21cc19 100644
>> --- a/Documentation/diff-options.txt
>> +++ b/Documentation/diff-options.txt
>> @@ -500,6 +500,10 @@ information.
>>  --pickaxe-regex::
>>  Treat the  given to `-S` as an extended POSIX regular
>>  expression to match.
>> +--blobfind=::
>> +Restrict the output such that one side of the diff
>> +matches the given blob-id.
>> +
>>  endif::git-format-patch[]
>
> Can we have a blank line between these enumerations to make the
> source easier to read?  Thanks.
>
> ...
> So, we keep an unmerged pair, a pair that mentions a sought-blob on
> one side or the other side?  I am not sure if we want to keep the
> unmerged pair for the purpose of this one.
>
>> +diff_free_filepair(p);
>> +q->queue[i] = NULL;
>> +c--;
>
> Also, if you are doing the in-place shrinking and have already
> introduced another counter 'j' that is initialized to 0, I think it
> makes more sense to do the shrinking in-place.  'i' will stay to be
> the source-scan pointer that runs 0 thru q->nr, while 'j' can be
> used in this loop (where you have 'continue') to move the current
> one that is determined to survive from q->queue[i] to q->queue[j++].
>
> Then you do not need 'c'; when the loop ends, 'j' would be the
> number of surviving entries and q->nr can be adjusted to it.  Unlike
> the usual pattern taken by the other diffcore transformations where
> a new queue is populated and the old one discarded, this would leave
> the q->queue[] over-allocated, but I do not think it is too bad.

Here is to illustrate the last point.  I still think we should keep
the unmerged entries for the purpose of blobfind but it should be
trivial to fix that.

 diffcore-blobfind.c | 33 -
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/diffcore-blobfind.c b/diffcore-blobfind.c
index 5d222fc336..bf63ba61dc 100644
--- a/diffcore-blobfind.c
+++ b/diffcore-blobfind.c
@@ -8,40 +8,31 @@
 static void diffcore_filter_blobs(struct diff_queue_struct *q,
  struct diff_options *options)
 {
-   int i, j = 0, c = q->nr;
+   int src, dst;
 
if (!options->blobfind)
BUG("blobfind oidset not initialized???");
 
-   for (i = 0; i < q->nr; i++) {
-   struct diff_filepair *p = q->queue[i];
+   for (src = dst = 0; src < q->nr; src++) {
+   struct diff_filepair *p = q->queue[src];
 
if (DIFF_PAIR_UNMERGED(p) ||
(DIFF_FILE_VALID(p->one) &&
 oidset_contains(options->blobfind, >one->oid)) ||
(DIFF_FILE_VALID(p->two) &&
-oidset_contains(options->blobfind, >two->oid)))
-   continue;
-
-   diff_free_filepair(p);
-   q->queue[i] = NULL;
-   c--;
-   }
-
-   /* Keep it sorted. */
-   i = 0; j = 0;
-   while (i < c) {
-   while (!q->queue[j])
-   j++;
-   q->queue[i] = q->queue[j];
-   i++; j++;
+oidset_contains(options->blobfind, >two->oid))) {
+   q->queue[dst] = p;
+   dst++;
+   } else {
+   diff_free_filepair(p);
+   }
}
 
-   q->nr = c;
-
-   if (!c) {
+   if (!dst) {
free(q->queue);
DIFF_QUEUE_CLEAR(q);
+   } else {
+   q->nr = dst;
}
 }
 

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Todd Zullinger]

Jeff Hostetler wrote:

I'm looking at t5616 now on my mac.
Looks like the MAC doesn't like my line counting in the tests.
I'll fix in my next version.


Perhaps that's as simple as using the test_line_counts helper?

diff --git i/t/t5616-partial-clone.sh w/t/t5616-partial-clone.sh
index fa573f8cdb..6d673de90c 100755
--- i/t/t5616-partial-clone.sh
+++ w/t/t5616-partial-clone.sh
@@ -19,7 +19,7 @@ test_expect_success 'setup normal src repo' '
git -C src ls-files -s file.$n.txt >>temp
done &&
awk -f print_2.awk expect_1.oids &&
-   test "$(wc -l expect.blame
'

@@ -100,7 +100,7 @@ test_expect_success 'partial fetch inherits filter 
settings' '
# it should be in a new packfile (since the promisor boundary is
# currently a packfile, it should not get unpacked upon receipt.)
test_expect_success 'verify diff causes dynamic object fetch' '
-   test "$(wc -l observed.oids &&
cat observed.oids &&
-   test "$(wc -l 

Re: [PATCH] strbuf: release memory on read error in strbuf_read_once()

[Jeff King]

On Thu, Dec 07, 2017 at 09:51:26PM +0100, René Scharfe wrote:

> If other strbuf add functions cause the first allocation and
> subsequently encounter an error then they release the memory, restoring
> the pristine state of the strbuf.  That simplifies error handling for
> callers.
> 
> Do the same in strbuf_read_once(), and do it also in case no bytes were
> read -- which may or may not be an error as well, depending on the
> caller.

Makes sense, and the patch is delightfully simple.

For the "0" case nobody should be negatively impacted by dropping the
allocation, as they get a sane 0-length string from the slopbuf (and
anybody who relies on sb->buf being allocated without calling detach or
similar is already doing it wrong).

-Peff

Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()

[Jeff King]

On Thu, Dec 07, 2017 at 09:22:49PM +0100, René Scharfe wrote:

> Use string_list_append_nodup() instead of string_list_append() to hand
> over ownership of a detached strbuf and thus avoid leaking its memory.

Looks obviously correct (though one thing missing from the diff context
is whether "subjects" is set to DUP -- it is, which is good).

Grepping for "list_append.*detach" shows a few other possible cases in
transport-helper.c, which I think are leaks.

I wondered if it would be possible to write a coccinelle rule for this,
but I think it's not possible. Whether this is right depends on the
strdup_strings flag, which could change at runtime (though in practice
it doesn't).

-Peff

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Junio C Hamano]

Lars Schneider  writes:

>> The acl stuff hasn't changed for a long time and I do not think of a
>> reason offhand why the test should behave differently between say
>> 'maint' and 'pu', yet 'maint' is passing while 'pu' is not...
>
> My recent 657343a602 (travis-ci: move Travis CI code into dedicated scripts, 
> 2017-09-10) change might have broken that somehow...

But the puzzling thing is that commit is in 'maint' as well as 'pu'.

[PATCH] worktree: invoke post-checkout hook (unless --no-checkout)

[Eric Sunshine]

git-clone and git-checkout both invoke the post-checkout hook following
a successful checkout, yet git-worktree neglects to do so even though it
too "checks out" the worktree. Fix this oversight.

Implementation note: The newly-created worktree may reference a branch
or be detached. In the latter case, a commit lookup is performed, though
the result is used only in a boolean sense to (a) determine if the
commit actually exists, and (b) assign either the branch name or commit
ID to HEAD. Since the post-commit hook needs to know the ID of the
checked-out commit, the lookup now needs to be done in all cases, rather
than only when detached. Consequently, a new boolean is needed to handle
(b) since the lookup result itself can no longer perform that role.

Reported-by: Matthew K Gumbel 
Signed-off-by: Eric Sunshine 
---
 Documentation/githooks.txt |  3 ++-
 builtin/worktree.c | 22 --
 t/t2025-worktree-add.sh| 29 +
 3 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 0bb0042d8c..91eb297f7b 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -170,7 +170,8 @@ This hook cannot affect the outcome of 'git checkout'.
 
 It is also run after 'git clone', unless the --no-checkout (-n) option is
 used. The first parameter given to the hook is the null-ref, the second the
-ref of the new HEAD and the flag is always 1.
+ref of the new HEAD and the flag is always 1. Likewise for 'git worktree add'
+unless --no-checkout is used.
 
 This hook can be used to perform repository validity checks, auto-display
 differences from the previous HEAD if different, or set working dir metadata
diff --git a/builtin/worktree.c b/builtin/worktree.c
index ed043d5f1c..9591f10442 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -218,20 +218,21 @@ static int add_worktree(const char *path, const char 
*refname,
int counter = 0, len, ret;
struct strbuf symref = STRBUF_INIT;
struct commit *commit = NULL;
+   int is_branch = 0;
 
if (file_exists(path) && !is_empty_dir(path))
die(_("'%s' already exists"), path);
 
/* is 'refname' a branch or commit? */
if (!opts->detach && !strbuf_check_branch_ref(, refname) &&
-ref_exists(symref.buf)) { /* it's a branch */
+   ref_exists(symref.buf)) {
+   is_branch = 1;
if (!opts->force)
die_if_checked_out(symref.buf, 0);
-   } else { /* must be a commit */
-   commit = lookup_commit_reference_by_name(refname);
-   if (!commit)
-   die(_("invalid reference: %s"), refname);
}
+   commit = lookup_commit_reference_by_name(refname);
+   if (!commit)
+   die(_("invalid reference: %s"), refname);
 
name = worktree_basename(path, );
git_path_buf(_repo, "worktrees/%.*s", (int)(path + len - name), 
name);
@@ -296,7 +297,7 @@ static int add_worktree(const char *path, const char 
*refname,
argv_array_pushf(_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
cp.git_cmd = 1;
 
-   if (commit)
+   if (!is_branch)
argv_array_pushl(, "update-ref", "HEAD",
 oid_to_hex(>object.oid), NULL);
else
@@ -327,6 +328,15 @@ static int add_worktree(const char *path, const char 
*refname,
strbuf_addf(, "%s/locked", sb_repo.buf);
unlink_or_warn(sb.buf);
}
+
+   /*
+* Hook failure does not warrant worktree deletion, so run hook after
+* is_junk is cleared, but do return appropriate code when hook fails.
+*/
+   if (!ret && opts->checkout)
+   ret = run_hook_le(NULL, "post-checkout", oid_to_hex(_oid),
+ oid_to_hex(>object.oid), "1", NULL);
+
argv_array_clear(_env);
strbuf_release();
strbuf_release();
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index b5c47ac602..d6fb062f83 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -314,4 +314,33 @@ test_expect_success 'rename a branch under bisect not 
allowed' '
test_must_fail git branch -M under-bisect bisect-with-new-name
 '
 
+post_checkout_hook () {
+   test_when_finished "rm -f .git/hooks/post-checkout" &&
+   mkdir -p .git/hooks &&
+   write_script .git/hooks/post-checkout <<-\EOF
+   echo $* >hook.actual
+   EOF
+}
+
+test_expect_success '"add" invokes post-checkout hook (branch)' '
+   post_checkout_hook &&
+   printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect &&
+   git worktree add gumby &&
+   test_cmp hook.expect hook.actual
+'
+
+test_expect_success '"add" invokes post-checkout hook (detached)' '
+   post_checkout_hook &&
+   

Re: [PATCH] am: release strbuf after use in split_mail_mbox()

[Jeff King]

On Thu, Dec 07, 2017 at 09:20:19PM +0100, René Scharfe wrote:

> Signed-off-by: Rene Scharfe 
> ---
>  builtin/am.c | 10 +++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/am.c b/builtin/am.c
> index 02853b3e05..1ac044da2e 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -708,6 +708,7 @@ static int split_mail_mbox(struct am_state *state, const 
> char **paths,
>  {
>   struct child_process cp = CHILD_PROCESS_INIT;
>   struct strbuf last = STRBUF_INIT;
> + int ret;
>  
>   cp.git_cmd = 1;
>   argv_array_push(, "mailsplit");
> @@ -721,13 +722,16 @@ static int split_mail_mbox(struct am_state *state, 
> const char **paths,
>   argv_array_push(, "--");
>   argv_array_pushv(, paths);
>  
> - if (capture_command(, , 8))
> - return -1;
> + ret = capture_command(, , 8);
> + if (ret)
> + goto exit;

Looks good to me.

Coupled with your third patch, it made me wonder if capture_command()
should free the strbuf when it sees an error. But probably not. Some
callers would want to see the output even from a failing command (and
doubly for pipe_command(), which may capture stderr).

(And anyway, it wouldn't make this case any simpler; we were leaking in
the success code path, too!)

-Peff

Re: [PATCH] diff: add tests for --relative without optional prefix value

[Jacob Keller]

On Thu, Dec 7, 2017 at 1:12 PM, Jeff King  wrote:
> On Thu, Dec 07, 2017 at 11:01:35AM -0800, Jacob Keller wrote:
>
>> From: Jacob Keller 
>>
>> We already have tests for --relative, but they currently only test when
>> a prefix has been provided. This fails to test the case where --relative
>> by itself should use the current directory as the prefix.
>>
>> Teach the check_$type functions to take a directory argument to indicate
>> which subdirectory to run the git commands in. Add a new test which uses
>> this to test --relative without a prefix value.
>
> This looks good to me (and I slightly prefer it over Junio's for the
> simplicity).
>
> I agree on the ordering suggestion Junio made.
>
> -Peff

As do I. Junio, if we go with my version, feel free to squash in the
re-order. Or if you prefer, I can send a v2 (though for such a small
change I don't see the benefit).

Thanks,
Jake

Re: [PATCH] diff: add tests for --relative without optional prefix value

[Jeff King]

On Thu, Dec 07, 2017 at 11:01:35AM -0800, Jacob Keller wrote:

> From: Jacob Keller 
> 
> We already have tests for --relative, but they currently only test when
> a prefix has been provided. This fails to test the case where --relative
> by itself should use the current directory as the prefix.
> 
> Teach the check_$type functions to take a directory argument to indicate
> which subdirectory to run the git commands in. Add a new test which uses
> this to test --relative without a prefix value.

This looks good to me (and I slightly prefer it over Junio's for the
simplicity).

I agree on the ordering suggestion Junio made.

-Peff

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Junio C Hamano]

Jeff Hostetler  writes:

> I'm looking at t5616 now on my mac.
> Looks like the MAC doesn't like my line counting in the tests.

Ah, of course, test "$(wc -l)" = number would not work over there
we have "test_line_count" helper exactly for that purose.

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Jeff King]

On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:

> Signed-off-by: Junio C Hamano 
> ---

It might be worth mentioning why this conversion is pulled out from the
others (because its "default" case is "do not touch the pointer").

Other than that, it looks good to me.

-Peff

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Jeff Hostetler]

On 12/7/2017 10:48 AM, Johannes Schindelin wrote:

Hi,

[...]


That is not the only thing going wrong:

https://travis-ci.org/git/git/builds/312551566

It would seem that t9001 is broken on Linux32, t5616 is broken on macOS,
and something really kinky is going on with the GETTEXT_POISON text, as it
seems to just abort while trying to run t6120.

Ciao,
Johannes



I'm looking at t5616 now on my mac.
Looks like the MAC doesn't like my line counting in the tests.
I'll fix in my next version.

Jeff

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Lars Schneider]

> On 07 Dec 2017, at 21:50, Junio C Hamano  wrote:
> 
> Todd Zullinger  writes:
> 
>> Johannes Schindelin wrote:
>>> That is not the only thing going wrong:
>>> 
>>> https://travis-ci.org/git/git/builds/312551566
>>> 
>>> It would seem that t9001 is broken on Linux32, t5616 is broken on macOS,
>>> and something really kinky is going on with the GETTEXT_POISON text, as it
>>> seems to just abort while trying to run t6120.
>> 
>> I thought the verbose logs from the test might be useful, but looking
>> at the travis output for that job[1], there's an unrelated problem
>> preventing the ci/print-test-failures.sh script from running properly:
>> 
>>   $ ci/print-test-failures.sh
>>   cat: t/test-results/t1304-default-acl.exit: Permission denied
>>   
>>   t/test-results/t1304-default-acl.out...
>>   
>>   cat: t/test-results/t1304-default-acl.out: Permission denied
>> 
>> [1] https://travis-ci.org/git/git/jobs/312551595#L2185
>> 
>> I didn't see the same failure for other build targets at a glance, so
>> the permission issue might only be a problem for the linux32 builds.
> 
> Curious.  
> 
> The acl stuff hasn't changed for a long time and I do not think of a
> reason offhand why the test should behave differently between say
> 'maint' and 'pu', yet 'maint' is passing while 'pu' is not...


My recent 657343a602 (travis-ci: move Travis CI code into dedicated scripts, 
2017-09-10) change might have broken that somehow...

See this comment:

# If this script runs inside a docker container, then all commands are
# usually executed as root. Consequently, the host user might not be
# able to access the test output files.
# If a host user id is given, then create a user "ci" with the host user
# id to make everything accessible to the host user.

https://github.com/git/git/blob/95ec6b1b3393eb6e26da40c565520a8db9796e9f/ci/run-linux32-build.sh#L16-L20

- Lars

Re: [WIP 01/15] pkt-line: introduce packet_read_with_status

[Stefan Beller]

On Mon, Dec 4, 2017 at 3:58 PM, Brandon Williams  wrote:

> diff --git a/pkt-line.h b/pkt-line.h
> index 3dad583e2..f1545929b 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -60,8 +60,16 @@ int write_packetized_from_buf(const char *src_in, size_t 
> len, int fd_out);
>   * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if
>   * present) is removed from the buffer before returning.
>   */
> +enum packet_read_status {
> +   PACKET_READ_ERROR = -1,
> +   PACKET_READ_NORMAL,
> +   PACKET_READ_FLUSH,
> +};
>  #define PACKET_READ_GENTLE_ON_EOF (1u<<0)
>  #define PACKET_READ_CHOMP_NEWLINE (1u<<1)
> +enum packet_read_status packet_read_with_status(int fd, char **src_buffer, 
> size_t *src_len,
> +   char *buffer, unsigned size, 
> int *pktlen,
> +   int options);
>  int packet_read(int fd, char **src_buffer, size_t *src_len, char
> *buffer, unsigned size, int options);

The documentation that is preceding these lines is very specific to
packet_read, e.g.

If options does contain PACKET_READ_GENTLE_ON_EOF,
we will not die on condition 4 (truncated input), but instead return -1

which doesn't hold true for the _status version. Can you adapt the comment
to explain both read functions?

[PATCH] strbuf: release memory on read error in strbuf_read_once()

[René Scharfe]

If other strbuf add functions cause the first allocation and
subsequently encounter an error then they release the memory, restoring
the pristine state of the strbuf.  That simplifies error handling for
callers.

Do the same in strbuf_read_once(), and do it also in case no bytes were
read -- which may or may not be an error as well, depending on the
caller.

Signed-off-by: Rene Scharfe 
---
 strbuf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index 323c49ceb3..ac5a7ab62d 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -386,12 +386,15 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t 
hint)
 
 ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
 {
+   size_t oldalloc = sb->alloc;
ssize_t cnt;
 
strbuf_grow(sb, hint ? hint : 8192);
cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
if (cnt > 0)
strbuf_setlen(sb, sb->len + cnt);
+   else if (oldalloc == 0)
+   strbuf_release(sb);
return cnt;
 }
 
-- 
2.15.1

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Junio C Hamano]

Todd Zullinger  writes:

> Johannes Schindelin wrote:
>> That is not the only thing going wrong:
>>
>>  https://travis-ci.org/git/git/builds/312551566
>>
>> It would seem that t9001 is broken on Linux32, t5616 is broken on macOS,
>> and something really kinky is going on with the GETTEXT_POISON text, as it
>> seems to just abort while trying to run t6120.
>
> I thought the verbose logs from the test might be useful, but looking
> at the travis output for that job[1], there's an unrelated problem
> preventing the ci/print-test-failures.sh script from running properly:
>
>$ ci/print-test-failures.sh
>cat: t/test-results/t1304-default-acl.exit: Permission denied
>
>t/test-results/t1304-default-acl.out...
>
>cat: t/test-results/t1304-default-acl.out: Permission denied
>
> [1] https://travis-ci.org/git/git/jobs/312551595#L2185
>
> I didn't see the same failure for other build targets at a glance, so
> the permission issue might only be a problem for the linux32 builds.

Curious.  

The acl stuff hasn't changed for a long time and I do not think of a
reason offhand why the test should behave differently between say
'maint' and 'pu', yet 'maint' is passing while 'pu' is not...

I wouldn't be surprised if Git::Error change is causing t9001
failure, as that can explain why 'pu' is different.

Re: [PATCH] RelNotes/2.16: Fix submodule recursing argument

[Junio C Hamano]

Stefan Beller  writes:

> Junio, feel free to just squash this into a future update
> of the release notes.
> ...
> - * "git checkout --recursive" may overwrite and rewind the history of
> + * "git checkout --recurse-submodules" may overwrite and rewind the history 
> of

Thanks.

Re: Feature request: Reduce amount of diff in patch

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Nov 28, 2017 at 8:09 AM, KES  wrote:
> ...
>> May you please fix the git to generate minimized patches?
>
> You can use a different diff algorithm.
> ...
> Soon we'll have another diff algorithm "anchor" that tries to
> keep a given line out of the +/- but rather move other lines around
> the line to give equal results.

While all of the above is correct, my understanding is that none of
them would produce a patch that KES wants to see.

And we shouldn't ;-)  The two changes KES shows in the message are
not equivalent.

[PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()

[René Scharfe]

Use string_list_append_nodup() instead of string_list_append() to hand
over ownership of a detached strbuf and thus avoid leaking its memory.

Signed-off-by: Rene Scharfe 
---
 builtin/fmt-merge-msg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 22034f87e7..8e8a15ea4a 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -377,7 +377,8 @@ static void shortlog(const char *name,
string_list_append(,
   oid_to_hex(>object.oid));
else
-   string_list_append(, strbuf_detach(, NULL));
+   string_list_append_nodup(,
+strbuf_detach(, NULL));
}
 
if (opts->credit_people)
-- 
2.15.1

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Todd Zullinger]

Johannes Schindelin wrote:

That is not the only thing going wrong:

https://travis-ci.org/git/git/builds/312551566

It would seem that t9001 is broken on Linux32, t5616 is broken on macOS,
and something really kinky is going on with the GETTEXT_POISON text, as it
seems to just abort while trying to run t6120.


I thought the verbose logs from the test might be useful, but looking
at the travis output for that job[1], there's an unrelated problem
preventing the ci/print-test-failures.sh script from running properly:

   $ ci/print-test-failures.sh
   cat: t/test-results/t1304-default-acl.exit: Permission denied
   
   t/test-results/t1304-default-acl.out...
   
   cat: t/test-results/t1304-default-acl.out: Permission denied

[1] https://travis-ci.org/git/git/jobs/312551595#L2185

I didn't see the same failure for other build targets at a glance, so
the permission issue might only be a problem for the linux32 builds.

--
Todd
~~
A diplomat is a person who can tell you to go to Hell in such a way
that you actually look forward to the trip.
   -- Anonymous

[PATCH] am: release strbuf after use in split_mail_mbox()

[René Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/am.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 02853b3e05..1ac044da2e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -708,6 +708,7 @@ static int split_mail_mbox(struct am_state *state, const 
char **paths,
 {
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf last = STRBUF_INIT;
+   int ret;
 
cp.git_cmd = 1;
argv_array_push(, "mailsplit");
@@ -721,13 +722,16 @@ static int split_mail_mbox(struct am_state *state, const 
char **paths,
argv_array_push(, "--");
argv_array_pushv(, paths);
 
-   if (capture_command(, , 8))
-   return -1;
+   ret = capture_command(, , 8);
+   if (ret)
+   goto exit;
 
state->cur = 1;
state->last = strtol(last.buf, NULL, 10);
 
-   return 0;
+exit:
+   strbuf_release();
+   return ret ? -1 : 0;
 }
 
 /**
-- 
2.15.1

Re: Feature request: Reduce amount of diff in patch

[Stefan Beller]

On Tue, Nov 28, 2017 at 8:09 AM, KES  wrote:
> Hi.
>
> I get often patches which can be minimized:
>
> @@ -60,11 +64,8 @@ sub _get_filter {
>  address=>  { -like => \[ '?',  "%$search%" ] },
>  company=>  { -like => \[ '?',  "%$search%" ] },
>  country_code =>  { '=' => \[ 'UPPER(?)' => $search ] },
> -]);
>
> -$users =  $users->search( $filter, {
> -prefetch => { Packages => { Ips => { Subnet => { Server => 
> 'Locality' ,
> -});
> +]);
>
>
>  return $users;
>
> This patch can be minimized to:
>
> @@ -60,11 +64,8 @@ sub _get_filter {
>  address=>  { -like => \[ '?',  "%$search%" ] },
>  company=>  { -like => \[ '?',  "%$search%" ] },
>  country_code =>  { '=' => \[ 'UPPER(?)' => $search ] },
>  ]);
>
> -$users =  $users->search( $filter, {
> -prefetch => { Packages => { Ips => { Subnet => { Server => 
> 'Locality' ,
> -});
>
>
>  return $users;
>
> May you please fix the git to generate minimized patches?

You can use a different diff algorithm.

   --diff-algorithm={patience|minimal|histogram|myers}
   Choose a diff algorithm. The variants are as follows:

   default, myers
   The basic greedy diff algorithm. Currently, this is the
   default.

   minimal
   Spend extra time to make sure the smallest possible diff is
   produced.

   patience
   Use "patience diff" algorithm when generating patches.

   histogram
   This algorithm extends the patience algorithm to "support
   low-occurrence common elements".

   For instance, if you configured diff.algorithm variable to a
   non-default value and want to use the default one, then you have to
   use --diff-algorithm=default option.

Soon we'll have another diff algorithm "anchor" that tries to
keep a given line out of the +/- but rather move other lines around
the line to give equal results.

Re: [PATCH] diff: add tests for --relative without optional prefix value

[Jacob Keller]

On Thu, Dec 7, 2017 at 11:17 AM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>>  for type in diff numstat stat raw; do
>> - check_$type file2 --relative=subdir/
>> - check_$type file2 --relative=subdir
>> - check_$type dir/file2 --relative=sub
>> + check_$type . file2 --relative=subdir/
>> + check_$type . file2 --relative=subdir
>> + check_$type . dir/file2 --relative=sub
>> + check_$type subdir file2 --relative
>
> OK, I didn't think it would be sensible to unconditionally pass the
> directory and use "-C ." as a no-op.  It looks good.
>
> I think the new one should go before the dir/file2 test; all three
> earlier tests (including this new one) are about taking a patch
> relative to subdir/ spelled in different ways, and the one about
> dir/file2 alone is different.

Yea, your patch looked fine to me, minus the use of subshells where we
could avoid it.

I'm fine taking your version.

Thanks,
Jake

[PATCH] RelNotes/2.16: Fix submodule recursing argument

[Stefan Beller]

Some commands take a plain `--recursive` flag as an indication to recurse
into submodules, git-clone is a notable user facing example, an internal
example is in builtin/submodule--helper. Other commands such as git-merge
take the `--recursive` flag to indicate recursing in their specific area
of expertise.

Given these examples it is evident, that such a flag is too generic as
we can think of other recursive applications as well: recursing into trees
is another example.

That is why any submodule related recursing tries to use the explicit
`--recurse-submodules` instead. Any occurrences of the genric recurse flag
are historic accidents.

Signed-off-by: Stefan Beller 
---

Junio, feel free to just squash this into a future update
of the release notes.

Thanks,
Stefan

 Documentation/RelNotes/2.16.0.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/RelNotes/2.16.0.txt 
b/Documentation/RelNotes/2.16.0.txt
index 431bd5e34a..8fbc233e56 100644
--- a/Documentation/RelNotes/2.16.0.txt
+++ b/Documentation/RelNotes/2.16.0.txt
@@ -275,7 +275,7 @@ Fixes since v2.15
ask the underlying "git fetch" to go over IPv4/IPv6, which has been
corrected.
 
- * "git checkout --recursive" may overwrite and rewind the history of
+ * "git checkout --recurse-submodules" may overwrite and rewind the history of
the branch that happens to be checked out in submodule
repositories, which might not be desirable.  Detach the HEAD but
still allow the recursive checkout to succeed in such a case.
-- 
2.15.1.424.g9478a66081-goog

Re: [PATCH v6 09/12] fixup: sha1_file: convert gotos to break/continue

[Brandon Williams]

On 12/05, Jeff Hostetler wrote:
> From: Jeff Hostetler 
> 

I'm a fan of eliminating looping goto statements.  I understand their
need for doing cleanup, but I think they should be reserved for that
specific case.  Thanks for cleaning this up!

> Signed-off-by: Jeff Hostetler 
> ---
>  sha1_file.c | 40 
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/sha1_file.c b/sha1_file.c
> index fc7718a..ce67f27 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1180,30 +1180,30 @@ int sha1_object_info_extended(const unsigned char 
> *sha1, struct object_info *oi,
>   }
>   }
>  
> -retry:
> - if (find_pack_entry(real, ))
> - goto found_packed;
> + while (1) {
> + if (find_pack_entry(real, ))
> + break;
>  
> - /* Most likely it's a loose object. */
> - if (!sha1_loose_object_info(real, oi, flags))
> - return 0;
> + /* Most likely it's a loose object. */
> + if (!sha1_loose_object_info(real, oi, flags))
> + return 0;
>  
> - /* Not a loose object; someone else may have just packed it. */
> - reprepare_packed_git();
> - if (find_pack_entry(real, ))
> - goto found_packed;
> -
> - /* Check if it is a missing object */
> - if (fetch_if_missing && repository_format_partial_clone &&
> - !already_retried) {
> - fetch_object(repository_format_partial_clone, real);
> - already_retried = 1;
> - goto retry;
> - }
> + /* Not a loose object; someone else may have just packed it. */
> + reprepare_packed_git();
> + if (find_pack_entry(real, ))
> + break;
>  
> - return -1;
> + /* Check if it is a missing object */
> + if (fetch_if_missing && repository_format_partial_clone &&
> + !already_retried) {
> + fetch_object(repository_format_partial_clone, real);
> + already_retried = 1;
> + continue;
> + }
> +
> + return -1;
> + }
>  
> -found_packed:
>   if (oi == _oi)
>   /*
>* We know that the caller doesn't actually need the
> -- 
> 2.9.3
> 

-- 
Brandon Williams

Re: [PATCH v6 03/12] fsck: support refs pointing to promisor objects

[Brandon Williams]

On 12/07, Jonathan Tan wrote:
> On Thu, 7 Dec 2017 11:18:52 -0800
> Brandon Williams  wrote:
> 
> > Instead of requiring that every test first removes 'repo', maybe you
> > want to have each test do its own cleanup by adding in
> > 'test_when_finished' lines to do the removals?  Just a thought.
> 
> If "test_when_finished" is the style that we plan to use in the project,
> we can do that.
> 
> But I think the "rm -rf" at the beginning of a test method is better
> than "test_when_finished", though. It makes the test independent (for
> example, the addition or removal of tests before such a test is less
> likely to affect that test), and makes it clear if (and how) the test
> does its own setup as opposed to requiring the setup from another test
> block.

You're right, at the end of the day you're just shuffling responsibility
to of cleanup somewhere else.

-- 
Brandon Williams

Re: [WIP 08/15] connect: discover protocol version outside of get_remote_heads

[Junio C Hamano]

Brandon Williams  writes:

> While we could wrap the preamble into a function it sort of defeats the
> purpose since you want to be able to call different functions based on
> the protocol version you're speaking.  That way you can have hard
> separations between the code paths which operate on v0/v1 and v2.

As I envision that the need for different processing across protocol
versions in real code would become far greater than it would fit in
cases within a single switch() statement, I imagined that the reader
structure would have a field that says which version of the protocol
it is, so that the caller of the preamble thing can inspect it later
to switch on it.

Re: [PATCH v6 03/12] fsck: support refs pointing to promisor objects

[Jonathan Tan]

On Thu, 7 Dec 2017 11:18:52 -0800
Brandon Williams  wrote:

> Instead of requiring that every test first removes 'repo', maybe you
> want to have each test do its own cleanup by adding in
> 'test_when_finished' lines to do the removals?  Just a thought.

If "test_when_finished" is the style that we plan to use in the project,
we can do that.

But I think the "rm -rf" at the beginning of a test method is better
than "test_when_finished", though. It makes the test independent (for
example, the addition or removal of tests before such a test is less
likely to affect that test), and makes it clear if (and how) the test
does its own setup as opposed to requiring the setup from another test
block.

Re: [PATCH v6 03/12] fsck: support refs pointing to promisor objects

[Brandon Williams]

On 12/05, Jeff Hostetler wrote:
> From: Jonathan Tan 
> 
> Teach fsck to not treat refs referring to missing promisor objects as an
> error when extensions.partialclone is set.
> 
> For the purposes of warning about no default refs, such refs are still
> treated as legitimate refs.
> 
> Signed-off-by: Jonathan Tan 
> ---
>  builtin/fsck.c   |  8 
>  t/t0410-partial-clone.sh | 24 
>  2 files changed, 32 insertions(+)
> 
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 2934299..ee937bb 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -434,6 +434,14 @@ static int fsck_handle_ref(const char *refname, const 
> struct object_id *oid,
>  
>   obj = parse_object(oid);
>   if (!obj) {
> + if (is_promisor_object(oid)) {
> + /*
> +  * Increment default_refs anyway, because this is a
> +  * valid ref.
> +  */
> +  default_refs++;
> +  return 0;
> + }
>   error("%s: invalid sha1 pointer %s", refname, oid_to_hex(oid));
>   errors_found |= ERROR_REACHABLE;
>   /* We'll continue with the rest despite the error.. */
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index 3ddb3b9..bf75162 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -13,6 +13,14 @@ pack_as_from_promisor () {
>   >repo/.git/objects/pack/pack-$HASH.promisor
>  }
>  
> +promise_and_delete () {
> + HASH=$(git -C repo rev-parse "$1") &&
> + git -C repo tag -a -m message my_annotated_tag "$HASH" &&
> + git -C repo rev-parse my_annotated_tag | pack_as_from_promisor &&
> + git -C repo tag -d my_annotated_tag &&
> + delete_object repo "$HASH"
> +}
> +
>  test_expect_success 'missing reflog object, but promised by a commit, passes 
> fsck' '
>   test_create_repo repo &&
>   test_commit -C repo my_commit &&
> @@ -78,4 +86,20 @@ test_expect_success 'missing reflog object alone fails 
> fsck, even with extension
>   test_must_fail git -C repo fsck
>  '
>  
> +test_expect_success 'missing ref object, but promised, passes fsck' '
> + rm -rf repo &&

Instead of requiring that every test first removes 'repo', maybe you
want to have each test do its own cleanup by adding in
'test_when_finished' lines to do the removals?  Just a thought.

> + test_create_repo repo &&
> + test_commit -C repo my_commit &&
> +
> + A=$(git -C repo commit-tree -m a HEAD^{tree}) &&
> +
> + # Reference $A only from ref
> + git -C repo branch my_branch "$A" &&
> + promise_and_delete "$A" &&
> +
> + git -C repo config core.repositoryformatversion 1 &&
> + git -C repo config extensions.partialclone "arbitrary string" &&
> + git -C repo fsck
> +'
> +
>  test_done
> -- 
> 2.9.3
> 

-- 
Brandon Williams

Re: [PATCH] diff: add tests for --relative without optional prefix value

[Junio C Hamano]

Jacob Keller  writes:

>  for type in diff numstat stat raw; do
> - check_$type file2 --relative=subdir/
> - check_$type file2 --relative=subdir
> - check_$type dir/file2 --relative=sub
> + check_$type . file2 --relative=subdir/
> + check_$type . file2 --relative=subdir
> + check_$type . dir/file2 --relative=sub
> + check_$type subdir file2 --relative

OK, I didn't think it would be sensible to unconditionally pass the
directory and use "-C ." as a no-op.  It looks good.

I think the new one should go before the dir/file2 test; all three
earlier tests (including this new one) are about taking a patch
relative to subdir/ spelled in different ways, and the one about
dir/file2 alone is different.

[PATCH] diff: add tests for --relative without optional prefix value

[Jacob Keller]

From: Jacob Keller 

We already have tests for --relative, but they currently only test when
a prefix has been provided. This fails to test the case where --relative
by itself should use the current directory as the prefix.

Teach the check_$type functions to take a directory argument to indicate
which subdirectory to run the git commands in. Add a new test which uses
this to test --relative without a prefix value.

Signed-off-by: Jacob Keller 
---
 t/t4045-diff-relative.sh | 19 ---
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 3950f5034d31..7d68a6e2a536 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -13,6 +13,7 @@ test_expect_success 'setup' '
 '
 
 check_diff() {
+dir=$1; shift
 expect=$1; shift
 cat >expected actual &&
test_cmp expected actual
 "
 }
 
 check_numstat() {
+dir=$1; shift
 expect=$1; shift
 cat >expected actual &&
+   git -C '$dir' diff --numstat $* HEAD^ >actual &&
test_cmp expected actual
 "
 }
 
 check_stat() {
+dir=$1; shift
 expect=$1; shift
 cat >expected actual &&
test_i18ncmp expected actual
 "
 }
 
 check_raw() {
+dir=$1; shift
 expect=$1; shift
 cat >expected actual &&
test_cmp expected actual
 "
 }
 
 for type in diff numstat stat raw; do
-   check_$type file2 --relative=subdir/
-   check_$type file2 --relative=subdir
-   check_$type dir/file2 --relative=sub
+   check_$type . file2 --relative=subdir/
+   check_$type . file2 --relative=subdir
+   check_$type . dir/file2 --relative=sub
+   check_$type subdir file2 --relative
 done
 
 test_done
-- 
2.15.1.478.ga1e07cd25f8b

Re: [PATCH v2 7/7] t4045: test 'diff --relative' for real

[Jacob Keller]

On Thu, Dec 7, 2017 at 9:30 AM, Junio C Hamano  wrote:
> The existing tests only checked how well -relative= work,
> without testing --relative (without any value).
>
> Signed-off-by: Junio C Hamano 
> ---
>  t/t4045-diff-relative.sh | 24 
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
> index fefd2f3f81..815cdd7295 100755
> --- a/t/t4045-diff-relative.sh
> +++ b/t/t4045-diff-relative.sh
> @@ -25,7 +25,10 @@ check_diff () {
> +other content
> EOF
> test_expect_success "-p $*" "
> -   git diff -p $* HEAD^ >actual &&
> +   (
> +   test -z "$in_there" || cd "$in_there"
> +   git diff -p $* HEAD^
> +   ) >actual &&
> test_cmp expected actual
> "
>  }
> @@ -38,7 +41,10 @@ check_numstat () {
> EOF
> test_expect_success "--numstat $*" "
> echo '1 0   $expect' >expected &&
> -   git diff --numstat $* HEAD^ >actual &&
> +   (
> +   test -z "$in_there" || cd "$in_there"
> +   git diff --numstat $* HEAD^
> +   ) >actual &&
> test_cmp expected actual
> "
>  }
> @@ -51,7 +57,10 @@ check_stat () {
>  1 file changed, 1 insertion(+)
> EOF
> test_expect_success "--stat $*" "
> -   git diff --stat $* HEAD^ >actual &&
> +   (
> +   test -z "$in_there" || cd "$in_there"
> +   git diff --stat $* HEAD^
> +   ) >actual &&
> test_i18ncmp expected actual
> "
>  }
> @@ -63,15 +72,22 @@ check_raw () {
> :00 100644  
> 25c05ef3639d2d270e7fe765a67668f098092bc5 A  $expect
> EOF
> test_expect_success "--raw $*" "
> -   git diff --no-abbrev --raw $* HEAD^ >actual &&
> +   (
> +   test -z "$in_there" || cd "$in_there"
> +   git diff --no-abbrev --raw $* HEAD^ >actual
> +   ) &&

You could avoid the subshell by just passing $in_there to -C on the
git commands. Same for the other tests. If you quote it, -C
'$in_there', then it will work regardless of if in_there is set or
not, (-C with an empty string doesn't cd anywhere). I think this is
generally preferable for tests given it helps avoid unnecessary
subshells when testing on Windows..?

> test_cmp expected actual
> "
>  }
>
>  for type in diff numstat stat raw
>  do
> +   in_there=
> check_$type file2 --relative=subdir/
> check_$type file2 --relative=subdir
> +   in_there=subdir
> +   check_$type file2 --relative
> +   in_there=
> check_$type dir/file2 --relative=sub
>  done
>

This isn't quite what I had in mind for the directory parameter. I
passed it as an extra argument, but I think this is probably more
sensible.

> --
> 2.15.1-480-gbc5668f98a
>

Re: [WIP 08/15] connect: discover protocol version outside of get_remote_heads

[Brandon Williams]

On 12/07, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > @@ -193,7 +195,17 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> > char *prefix)
> > if (!conn)
> > return args.diag_url ? 0 : 1;
> > }
> > -   get_remote_heads(fd[0], NULL, 0, , 0, NULL, );
> > +
> > +   packet_reader_init(, fd[0], NULL, 0);
> > +
> > +   switch (discover_version()) {
> > +   case protocol_v1:
> > +   case protocol_v0:
> > +   get_remote_heads(, , 0, NULL, );
> > +   break;
> > +   case protocol_unknown_version:
> > +   BUG("unknown protocol version");
> > +   }
> 
> We see quite a few hunks just like this one appear in this patch.
> The repetition is a bit disturbing.  I wonder if we want a wrapper
> around the "reader-init && discover-version && return an error if
> the protocol version is not known" dance.  That would allow us to
> write this part of the code like so:
> 
>   struct packet_reader reader;
> 
>   if (connection_preamble(, fd[0]))
>   die("unknown protocol version");
>   get_remote_heads(, , 0, NULL, );
> 
> or something like that.
> 
> By the way, is that really a BUG()?  Getting a connection and the
> other end declaring a protocol version you do not yet understand is
> not a bug in your program---it is a normal runtime error, no?

While we could wrap the preamble into a function it sort of defeats the
purpose since you want to be able to call different functions based on
the protocol version you're speaking.  That way you can have hard
separations between the code paths which operate on v0/v1 and v2.

As for that case being a BUG, yes it should be a BUG.  the
discover_version function won't ever return a protocol_unknown_version
value.  Its only there to make sure the switch cases are exhaustive and
cover all the enum values.  That does bring up a good point though.
This error should be handled as a run-time error and not a BUG in
discover_version, which I'll fix.


-- 
Brandon Williams

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Jacob Keller]

On Thu, Dec 7, 2017 at 9:30 AM, Junio C Hamano  wrote:
> Signed-off-by: Junio C Hamano 
> ---
>  diff.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index cd032c6367..e99ac6ec8a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4563,11 +4563,10 @@ int diff_opt_parse(struct diff_options *options,
> options->flags.rename_empty = 1;
> else if (!strcmp(arg, "--no-rename-empty"))
> options->flags.rename_empty = 0;
> -   else if (!strcmp(arg, "--relative"))
> +   else if (skip_to_optional_val_default(arg, "--relative", , NULL)) 
> {
> options->flags.relative_name = 1;
> -   else if (skip_prefix(arg, "--relative=", )) {
> -   options->flags.relative_name = 1;
> -   options->prefix = arg;
> +   if (arg)
> +   options->prefix = arg;
> }
>
> /* xdiff options */
> --
> 2.15.1-480-gbc5668f98a
>

Yea, this is exactly what I had imagined as the fix.

Thanks,
Jake

Re: [PATCH v2] diff: fix --relative without arguments

[Jacob Keller]

On Thu, Dec 7, 2017 at 7:26 AM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> From: Jacob Keller 
>>
>> Recently, commit f7923a5ece00 ("diff: use skip_to_optional_val()",
>> 2017-12-04) changed how we parsed some diff options, preferring
>> skip_to_optional_val instead of a more complex skip_prefix() setup.
>
> I'd expect a moral equivalent of this squashed in when Christian
> rerolls the skip-to-optional-val series (with helped-by: attribution
> to you, probably).  It does not make much sense to leave the initial
> breakage like this one in the history.
>
> Thanks.

Quite correct. I'll be sending a patch with just new tests shortly.

Thanks,
Jake

Re: git commit file completion recently broke

[Jacob Keller]

On Thu, Dec 7, 2017 at 7:24 AM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>>> I do think it may make sense for
>>> the "short" one to use NULL, like:
>>>
>>>   skip_to_optional_val(arg, "--relative, )
>>>
>>> but maybe some other callers would be more inconvenienced (they may have
>>> to current NULL back into the empty string if they want to string
>>> "--foo" the same as "--foo=").
>>
>> I discussed that with Junio and yeah there are many callers that want
>> "--foo" to be the same as "--foo=".
>
> Yup, the original thread has details and me saying that assuming all
> of them want --foo and --foo= the same is questionable.  The likely
> fix would be to use the _default variant with NULL, which was added
> exactly for cases like this.
>

Slightly more complex. You have to use the _default variant, pass in
arg instead of options->prefix, and then make sure arg was set before
overwriting options->prefix. If you just use _default with NULL, it
will not quite fix the problem.

Thanks,
Jake

Re: [WIP 08/15] connect: discover protocol version outside of get_remote_heads

[Junio C Hamano]

Brandon Williams  writes:

> @@ -193,7 +195,17 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> char *prefix)
>   if (!conn)
>   return args.diag_url ? 0 : 1;
>   }
> - get_remote_heads(fd[0], NULL, 0, , 0, NULL, );
> +
> + packet_reader_init(, fd[0], NULL, 0);
> +
> + switch (discover_version()) {
> + case protocol_v1:
> + case protocol_v0:
> + get_remote_heads(, , 0, NULL, );
> + break;
> + case protocol_unknown_version:
> + BUG("unknown protocol version");
> + }

We see quite a few hunks just like this one appear in this patch.
The repetition is a bit disturbing.  I wonder if we want a wrapper
around the "reader-init && discover-version && return an error if
the protocol version is not known" dance.  That would allow us to
write this part of the code like so:

struct packet_reader reader;

if (connection_preamble(, fd[0]))
die("unknown protocol version");
get_remote_heads(, , 0, NULL, );

or something like that.

By the way, is that really a BUG()?  Getting a connection and the
other end declaring a protocol version you do not yet understand is
not a bug in your program---it is a normal runtime error, no?

Re: [WIP 06/15] transport: use get_refs_via_connect to get refs

[Brandon Williams]

On 12/06, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > Remove code duplication and use the existing 'get_refs_via_connect()'
> > function to retrieve a remote's heads in 'fetch_refs_via_pack()' and
> > 'git_transport_push()'.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  transport.c | 18 --
> >  1 file changed, 4 insertions(+), 14 deletions(-)
> >
> > diff --git a/transport.c b/transport.c
> > index d75ff0514..7c969f285 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport 
> > *transport,
> > args.cloning = transport->cloning;
> > args.update_shallow = data->options.update_shallow;
> >  
> > -   if (!data->got_remote_heads) {
> > -   connect_setup(transport, 0);
> > -   get_remote_heads(data->fd[0], NULL, 0, _tmp, 0,
> > -NULL, >shallow);
> > -   data->got_remote_heads = 1;
> > -   }
> > +   if (!data->got_remote_heads)
> > +   refs_tmp = get_refs_via_connect(transport, 0);
> 
> The updated version is equivalent to the original as long as
> transport->data->extra_have is empty at this point.  Were we
> deliberately sending NULL, instead of >extra_have, in the
> original, or is it a mere oversight?
> 
> The same comment applies to the other hunk of this patch.

extra_have is only ever used by the push logic, so they're shouldn't be
any harm is passing it through on the fetch side, especially since
upload-pack doesn't send .have lines.

The push side is what uses the .have lines.  From a quick look through
the code it seems like get_refs_via_connect is always called before
git_transport_push, so the extra check to make sure ref's have been
retrieved doesn't get executed.  But if it ever did get executed, we
would silently ignore a server's .have lines.

-- 
Brandon Williams

What's cooking in git.git (Dec 2017, #02; Thu, 7)

[Junio C Hamano]

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

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

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

--
[Graduated to "master"]

* ac/complete-pull-autostash (2017-11-22) 1 commit
  (merged to 'next' on 2017-11-27 at 802d204eda)
 + completion: add --autostash and --no-autostash to pull

 The shell completion (in contrib/) learned that "git pull" can take
 the "--autostash" option.


* bw/protocol-v1 (2017-10-17) 11 commits
  (merged to 'next' on 2017-11-27 at 55040d09ec)
 + Documentation: document Extra Parameters
 + ssh: introduce a 'simple' ssh variant
 + i5700: add interop test for protocol transition
 + http: tell server that the client understands v1
 + connect: tell server that the client understands v1
 + connect: teach client to recognize v1 server response
 + upload-pack, receive-pack: introduce protocol version 1
 + daemon: recognize hidden request arguments
 + protocol: introduce protocol extension mechanisms
 + pkt-line: add packet_write function
 + connect: in ref advertisement, shallows are last
 (this branch is used by jn/ssh-wrappers.)

 A new mechanism to upgrade the wire protocol in place is proposed
 and demonstrated that it works with the older versions of Git
 without harming them.


* cc/git-packet-pm (2017-11-22) 2 commits
  (merged to 'next' on 2017-11-27 at 1527ab3519)
 + Git/Packet.pm: use 'if' instead of 'unless'
 + Git/Packet: clarify that packet_required_key_val_read allows EOF

 Code clean-up.


* cc/perf-run-config (2017-09-24) 9 commits
  (merged to 'next' on 2017-11-27 at d75a2469eb)
 + perf: store subsection results in "test-results/$GIT_PERF_SUBSECTION/"
 + perf/run: show name of rev being built
 + perf/run: add run_subsection()
 + perf/run: update get_var_from_env_or_config() for subsections
 + perf/run: add get_subsections()
 + perf/run: add calls to get_var_from_env_or_config()
 + perf/run: add GIT_PERF_DIRS_OR_REVS
 + perf/run: add get_var_from_env_or_config()
 + perf/run: add '--config' option to the 'run' script


* hm/config-parse-expiry-date (2017-11-18) 1 commit
  (merged to 'next' on 2017-11-27 at 20014f5541)
 + config: add --expiry-date

 "git config --expiry-date gc.reflogexpire" can read "2.weeks" from
 the configuration and report it as a timestamp, just like "--int"
 would read "1k" and report 1024, to help consumption by scripts.


* jk/fewer-pack-rescan (2017-11-22) 5 commits
  (merged to 'next' on 2017-11-27 at 2c35a2d831)
 + sha1_file: fast-path null sha1 as a missing object
 + everything_local: use "quick" object existence check
 + p5551: add a script to test fetch pack-dir rescans
 + t/perf/lib-pack: use fast-import checkpoint to create packs
 + p5550: factor out nonsense-pack creation

 Internaly we use 0{40} as a placeholder object name to signal the
 codepath that there is no such object (e.g. the fast-forward check
 while "git fetch" stores a new remote-tracking ref says "we know
 there is no 'old' thing pointed at by the ref, as we are creating
 it anew" by passing 0{40} for the 'old' side), and expect that a
 codepath to locate an in-core object to return NULL as a sign that
 the object does not exist.  A look-up for an object that does not
 exist however is quite costly with a repository with large number
 of packfiles.  This access pattern has been optimized.


* jn/reproducible-build (2017-11-22) 3 commits
  (merged to 'next' on 2017-11-27 at 6ae6946f8c)
 + Merge branch 'jn/reproducible-build' of ../git-gui into jn/reproducible-build
 + git-gui: sort entries in optimized tclIndex
 + generate-cmdlist: avoid non-deterministic output

 The build procedure has been taught to avoid some unnecessary
 instability in the build products.


* jn/ssh-wrappers (2017-11-21) 9 commits
  (merged to 'next' on 2017-11-27 at 00a2bb7a3c)
 + connect: correct style of C-style comment
 + ssh: 'simple' variant does not support --port
 + ssh: 'simple' variant does not support -4/-6
 + ssh: 'auto' variant to select between 'ssh' and 'simple'
 + connect: split ssh option computation to its own function
 + connect: split ssh command line options into separate function
 + connect: split git:// setup into a separate function
 + connect: move no_fork fallback to git_tcp_connect
 + ssh test: make copy_ssh_wrapper_as clean up after itself
 (this branch uses bw/protocol-v1.)

 The ssh-variant 'simple' introduced earlier broke existing
 installations by not passing --port/-4/-6 and not diagnosing an
 attempt to pass these as an error.  Instead, default to
 automatically detect how compatible the GIT_SSH/GIT_SSH_COMMAND is
 to OpenSSH convention and then error out an invocation to make it
 easier to 

Re: [PATCH v5 2/2] launch_editor(): indicate that Git waits for user input

[Lars Schneider]

> On 07 Dec 2017, at 18:37, Kaartic Sivaraam  wrote:
> 
> On Thursday 07 December 2017 10:00 PM, Junio C Hamano wrote:
>> +
>> +if (print_waiting_for_editor) {
>> +/*
>> + * A dumb terminal cannot erase the line later on. Add a
>> + * newline to separate the hint from subsequent output.
>> + *
> 
> 
>> + * In case the editor emits further cruft after what
>> + * we wrote above, separate it from our message with SP.
> 
> I guess this part of the comment could be improved a little. I currently 
> interpret it as "See if the editor emits further cruft, print a space in that 
> case". Though, it's not what we are doing. Something like the following, 
> perhaps?
> 
> In a non-dumb terminal, separate our message from further cruft
> that might be emitted by the editor with SP.

I see what you mean. My (non-native) language feeling tells me that
reordering the sentence might sound better:

 * In a non-dumb terminal, separate our message 
with SP
 * from further cruft that might be emitted by 
the editor.


@Junio: If you agree with the change, can you squash either of the new 
versions? 

Thanks,
Lars

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Johannes Schindelin]

Hi,

On Wed, 6 Dec 2017, Lars Schneider wrote:

> 
> > On 04 Dec 2017, at 22:46, Junio C Hamano  wrote:
> > 
> > 
> > * tb/check-crlf-for-safe-crlf (2017-11-27) 1 commit
> > - convert: tighten the safe autocrlf handling
> > 
> > The "safe crlf" check incorrectly triggered for contents that does
> > not use CRLF as line endings, which has been corrected.
> > 
> > Will merge to 'next'.
> > 
> 
> Looks like t0027-auto-crlf.sh is failing on Windows:
> https://travis-ci.org/git/git/jobs/312135514
> 
> Could that patch be related?

That is not the only thing going wrong:

https://travis-ci.org/git/git/builds/312551566

It would seem that t9001 is broken on Linux32, t5616 is broken on macOS,
and something really kinky is going on with the GETTEXT_POISON text, as it
seems to just abort while trying to run t6120.

Ciao,
Johannes

Re: [PATCH v5 2/2] launch_editor(): indicate that Git waits for user input

[Lars Schneider]

> On 07 Dec 2017, at 16:43, Junio C Hamano  wrote:
> 
> lars.schnei...@autodesk.com writes:
> 
>> +if (print_waiting_for_editor) {
>> +fprintf(stderr,
>> +_("hint: Waiting for your editor to close the 
>> file... "));
>> +if (is_terminal_dumb())
>> +/*
>> + * A dumb terminal cannot erase the line later 
>> on. Add a
>> + * newline to separate the hint from subsequent 
>> output.
>> + */
>> +fprintf(stderr, "\n");
>> +fflush(stderr);
>> +}
> 
> Was the trailing whitespace at the end of the hint message intended?
> 
> If we expect the editor to spit out additional garbage on the line,
> it would probably help to have that SP,

Argh. I forgot to mention that in the cover letter. Yes, I added
the whitespace intentionally for exactly that reason.


> but if that is why we have it
> there, it probably should be done only when !is_terminal_dumb().

That, of course, is correct. My intention was to make the code simpler
but I can see that people would be confused about the whitespace.

How about this?

fprintf(stderr,
_("hint: Waiting for your editor to close the 
file..."));
if (is_terminal_dumb())
/*
 * A dumb terminal cannot erase the line later 
on. Add a
 * newline to separate the hint from subsequent 
output.
 */
fprintf(stderr, "\n")
else
fprintf(stderr, " ")

Can you squash that if you like it?

Thanks,
Lars

Re: [PATCH v5 2/2] launch_editor(): indicate that Git waits for user input

[Kaartic Sivaraam]

On Thursday 07 December 2017 10:00 PM, Junio C Hamano wrote:

+
+   if (print_waiting_for_editor) {
+   /*
+* A dumb terminal cannot erase the line later on. Add a
+* newline to separate the hint from subsequent output.
+*




+* In case the editor emits further cruft after what
+* we wrote above, separate it from our message with SP.


I guess this part of the comment could be improved a little. I currently 
interpret it as "See if the editor emits further cruft, print a space in 
that case". Though, it's not what we are doing. Something like the 
following, perhaps?


 In a non-dumb terminal, separate our message from further cruft
 that might be emitted by the editor with SP.




+*/
+   const char term = is_terminal_dumb() ? '\n' : ' ';
+
+   fprintf(stderr,
+   _("hint: Waiting for your editor to close the 
file...%c"),
+   term);
+   fflush(stderr);
+   }
  
  		p.argv = args;

p.env = env;
@@ -63,6 +80,13 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
if (ret)
return error("There was a problem with the editor 
'%s'.",
editor);
+
+   if (print_waiting_for_editor && !is_terminal_dumb())
+   /*
+* Go back to the beginning and erase the entire line to
+* avoid wasting the vertical space.
+*/
+   fputs("\r\033[K", stderr);
}
  
  	if (!buffer)

[PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Junio C Hamano]

Signed-off-by: Junio C Hamano 
---
 diff.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index cd032c6367..e99ac6ec8a 100644
--- a/diff.c
+++ b/diff.c
@@ -4563,11 +4563,10 @@ int diff_opt_parse(struct diff_options *options,
options->flags.rename_empty = 1;
else if (!strcmp(arg, "--no-rename-empty"))
options->flags.rename_empty = 0;
-   else if (!strcmp(arg, "--relative"))
+   else if (skip_to_optional_val_default(arg, "--relative", , NULL)) {
options->flags.relative_name = 1;
-   else if (skip_prefix(arg, "--relative=", )) {
-   options->flags.relative_name = 1;
-   options->prefix = arg;
+   if (arg)
+   options->prefix = arg;
}
 
/* xdiff options */
-- 
2.15.1-480-gbc5668f98a

[PATCH v2 0/7] reroll of cc/skip-to-optional-val

[Junio C Hamano]

I'm sending out only the last three parts, as the changes necessary
to 03/07 that incorrectly used the variant without _default() to
overwrite options->prefix should be trivally obvious.

05/07 uses the _default() variant so that the caller can react
differently to "--relative" from "--relative=" correctly.

06/07 is an update to t4045, so that the change made in 07/07
becomes readable.

07/07 updates the test to verify the change in 05/07.  I just made
sure that these tests catch it if we deliberately reintroduce the
broken version from Christian's series on top.

Junio C Hamano (3):
  diff: use skip-to-optional-val in parsing --relative
  t4045: reindent to make helpers readable
  t4045: test 'diff --relative' for real

 builtin/index-pack.c |  11 ++---
 diff.c   |  50 +++--
 git-compat-util.h|  23 ++
 strbuf.c |  20 +
 t/t4045-diff-relative.sh | 111 ---
 5 files changed, 128 insertions(+), 87 deletions(-)

-- 
2.15.1-480-gbc5668f98a

[PATCH v2 7/7] t4045: test 'diff --relative' for real

[Junio C Hamano]

The existing tests only checked how well -relative= work,
without testing --relative (without any value).

Signed-off-by: Junio C Hamano 
---
 t/t4045-diff-relative.sh | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index fefd2f3f81..815cdd7295 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -25,7 +25,10 @@ check_diff () {
+other content
EOF
test_expect_success "-p $*" "
-   git diff -p $* HEAD^ >actual &&
+   (
+   test -z "$in_there" || cd "$in_there"
+   git diff -p $* HEAD^
+   ) >actual &&
test_cmp expected actual
"
 }
@@ -38,7 +41,10 @@ check_numstat () {
EOF
test_expect_success "--numstat $*" "
echo '1 0   $expect' >expected &&
-   git diff --numstat $* HEAD^ >actual &&
+   (
+   test -z "$in_there" || cd "$in_there"
+   git diff --numstat $* HEAD^
+   ) >actual &&
test_cmp expected actual
"
 }
@@ -51,7 +57,10 @@ check_stat () {
 1 file changed, 1 insertion(+)
EOF
test_expect_success "--stat $*" "
-   git diff --stat $* HEAD^ >actual &&
+   (
+   test -z "$in_there" || cd "$in_there"
+   git diff --stat $* HEAD^
+   ) >actual &&
test_i18ncmp expected actual
"
 }
@@ -63,15 +72,22 @@ check_raw () {
:00 100644  
25c05ef3639d2d270e7fe765a67668f098092bc5 A  $expect
EOF
test_expect_success "--raw $*" "
-   git diff --no-abbrev --raw $* HEAD^ >actual &&
+   (
+   test -z "$in_there" || cd "$in_there"
+   git diff --no-abbrev --raw $* HEAD^ >actual
+   ) &&
test_cmp expected actual
"
 }
 
 for type in diff numstat stat raw
 do
+   in_there=
check_$type file2 --relative=subdir/
check_$type file2 --relative=subdir
+   in_there=subdir
+   check_$type file2 --relative
+   in_there=
check_$type dir/file2 --relative=sub
 done
 
-- 
2.15.1-480-gbc5668f98a

[PATCH v2 6/7] t4045: reindent to make helpers readable

[Junio C Hamano]

Signed-off-by: Junio C Hamano 
---
 t/t4045-diff-relative.sh | 95 +---
 1 file changed, 50 insertions(+), 45 deletions(-)

diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 3950f5034d..fefd2f3f81 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -12,59 +12,64 @@ test_expect_success 'setup' '
git commit -m one
 '
 
-check_diff() {
-expect=$1; shift
-cat >expected expected <<-EOF
+   diff --git a/$expect b/$expect
+   new file mode 100644
+   index 000..25c05ef
+   --- /dev/null
+   +++ b/$expect
+   @@ -0,0 +1 @@
+   +other content
+   EOF
+   test_expect_success "-p $*" "
+   git diff -p $* HEAD^ >actual &&
+   test_cmp expected actual
+   "
 }
 
-check_numstat() {
-expect=$1; shift
-cat >expected actual &&
-   test_cmp expected actual
-"
+check_numstat () {
+   expect=$1
+   shift
+   cat >expected <<-EOF
+   1   0   $expect
+   EOF
+   test_expect_success "--numstat $*" "
+   echo '1 0   $expect' >expected &&
+   git diff --numstat $* HEAD^ >actual &&
+   test_cmp expected actual
+   "
 }
 
-check_stat() {
-expect=$1; shift
-cat >expected expected <<-EOF
+$expect | 1 +
+1 file changed, 1 insertion(+)
+   EOF
+   test_expect_success "--stat $*" "
+   git diff --stat $* HEAD^ >actual &&
+   test_i18ncmp expected actual
+   "
 }
 
-check_raw() {
-expect=$1; shift
-cat >expected expected <<-EOF
+   :00 100644  
25c05ef3639d2d270e7fe765a67668f098092bc5 A  $expect
+   EOF
+   test_expect_success "--raw $*" "
+   git diff --no-abbrev --raw $* HEAD^ >actual &&
+   test_cmp expected actual
+   "
 }
 
-for type in diff numstat stat raw; do
+for type in diff numstat stat raw
+do
check_$type file2 --relative=subdir/
check_$type file2 --relative=subdir
check_$type dir/file2 --relative=sub
-- 
2.15.1-480-gbc5668f98a

Re: git blame --reverse doesn't find line in HEAD

[Nick Snyder]

I built from source and was unable to find a git version where this
has ever worked correctly.

I wasn't able to compile and test versions older than 1.6.1.

Confirmed not working:
2.15.1
2.13.6 (Apple Git-96)
2.0.0
1.7.0
1.6.3
1.6.2
1.6.1

I updated the https://github.com/nicksnyder/git-blame-bug with a
script to easily reproduce.

On Wed, Dec 6, 2017 at 10:00 AM, Nick Snyder  wrote:
>> Can you bisect to see when the feature stopped working as you expect?
>
> I will see if I can do that but might take some time.
>
>> It finds up to which commit each line survived without getting touched since 
>> the oldest commit in the range.
>
> Right, this is where it is failing in my case.
>
> With a history like this:
> A <- B <- C <- HEAD
>
> I have a particular line in C (HEAD) that blames to commit A.
> If I run a git blame --reverse starting at commit A for that line, it
> doesn't give me back C, it gives me back B instead.
> The line is not added/deleted/moved between B and C.
>
>
>
> On Wed, Dec 6, 2017 at 9:22 AM, Junio C Hamano  wrote:
>> Nick Snyder  writes:
>>
>>> This can be reproduced on Linux and Mac. This behavior seems to be a bug.
>>
>> Can you bisect to see when the feature stopped working as you expect?
>>
>> Unlike a forward blame, where the command tries to find a commit
>> that is responsible for a line being in the final result (i.e.
>> typically, HEAD), a reverse blame is not about finding a commit
>> that is responsible for a line (that used to be in the oldest
>> commit) not being in a more recent codebase.  It finds up to which
>> commit each line survived without getting touched since the oldest
>> commit in the range.
>>

Re: [PATCH v5 2/2] launch_editor(): indicate that Git waits for user input

[Lars Schneider]

> On 07 Dec 2017, at 17:30, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>>> Can you squash that if you like it?
> 
> I thought you also had to update the log message that you forgot to?
> 
> Here is the replacement I queued tentatively.

Perfect. Thanks a lot for your additional fixes!

- Lars

> 
> -- >8 --
> From: Lars Schneider 
> Date: Thu, 7 Dec 2017 16:16:41 +0100
> Subject: [PATCH] launch_editor(): indicate that Git waits for user input
> 
> When a graphical GIT_EDITOR is spawned by a Git command that opens
> and waits for user input (e.g. "git rebase -i"), then the editor window
> might be obscured by other windows. The user might be left staring at
> the original Git terminal window without even realizing that s/he needs
> to interact with another window before Git can proceed. To this user Git
> appears hanging.
> 
> Print a message that Git is waiting for editor input in the original
> terminal and get rid of it when the editor returns, if the terminal
> supports erasing the last line.  Also, make sure that our message is
> terminated with a whitespace so that any message the editor may show
> upon starting up will be kept separate from our message.
> 
> Power users might not want to see this message or their editor might
> already print such a message (e.g. emacsclient). Allow these users to
> suppress the message by disabling the "advice.waitingForEditor" config.
> 
> The standard advise() function is not used here as it would always add
> a newline which would make deleting the message harder.
> 
> Helped-by: Junio C Hamano 
> Signed-off-by: Lars Schneider 
> Signed-off-by: Junio C Hamano 
> ---
> Documentation/config.txt |  3 +++
> advice.c |  2 ++
> advice.h |  1 +
> editor.c | 24 
> 4 files changed, 30 insertions(+)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 9593bfabaa..6ebc50eea8 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -351,6 +351,9 @@ advice.*::
>   addEmbeddedRepo::
>   Advice on what to do when you've accidentally added one
>   git repo inside of another.
> + waitingForEditor::
> + Print a message to the terminal whenever Git is waiting for
> + editor input from the user.
> --
> 
> core.fileMode::
> diff --git a/advice.c b/advice.c
> index d81e1cb742..af29d23e43 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -17,6 +17,7 @@ int advice_set_upstream_failure = 1;
> int advice_object_name_warning = 1;
> int advice_rm_hints = 1;
> int advice_add_embedded_repo = 1;
> +int advice_waiting_for_editor = 1;
> 
> static struct {
>   const char *name;
> @@ -38,6 +39,7 @@ static struct {
>   { "objectnamewarning", _object_name_warning },
>   { "rmhints", _rm_hints },
>   { "addembeddedrepo", _add_embedded_repo },
> + { "waitingforeditor", _waiting_for_editor },
> 
>   /* make this an alias for backward compatibility */
>   { "pushnonfastforward", _push_update_rejected }
> diff --git a/advice.h b/advice.h
> index c84a44531c..f7cbbd342f 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -19,6 +19,7 @@ extern int advice_set_upstream_failure;
> extern int advice_object_name_warning;
> extern int advice_rm_hints;
> extern int advice_add_embedded_repo;
> +extern int advice_waiting_for_editor;
> 
> int git_default_advice_config(const char *var, const char *value);
> __attribute__((format (printf, 1, 2)))
> diff --git a/editor.c b/editor.c
> index c65ea698eb..8acce0dcd4 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -45,6 +45,23 @@ int launch_editor(const char *path, struct strbuf *buffer, 
> const char *const *en
>   const char *args[] = { editor, real_path(path), NULL };
>   struct child_process p = CHILD_PROCESS_INIT;
>   int ret, sig;
> + int print_waiting_for_editor = advice_waiting_for_editor && 
> isatty(2);
> +
> + if (print_waiting_for_editor) {
> + /*
> +  * A dumb terminal cannot erase the line later on. Add a
> +  * newline to separate the hint from subsequent output.
> +  *
> +  * In case the editor emits further cruft after what
> +  * we wrote above, separate it from our message with SP.
> +  */
> + const char term = is_terminal_dumb() ? '\n' : ' ';
> +
> + fprintf(stderr,
> + _("hint: Waiting for your editor to close the 
> file...%c"),
> + term);
> + fflush(stderr);
> + }
> 
>   p.argv = args;
>   p.env = env;
> @@ -63,6 +80,13 @@ int launch_editor(const char *path, struct strbuf 

Re: [PATCH v5 2/2] launch_editor(): indicate that Git waits for user input

[Junio C Hamano]

Lars Schneider  writes:

>> Can you squash that if you like it?

I thought you also had to update the log message that you forgot to?

Here is the replacement I queued tentatively.

-- >8 --
From: Lars Schneider 
Date: Thu, 7 Dec 2017 16:16:41 +0100
Subject: [PATCH] launch_editor(): indicate that Git waits for user input

When a graphical GIT_EDITOR is spawned by a Git command that opens
and waits for user input (e.g. "git rebase -i"), then the editor window
might be obscured by other windows. The user might be left staring at
the original Git terminal window without even realizing that s/he needs
to interact with another window before Git can proceed. To this user Git
appears hanging.

Print a message that Git is waiting for editor input in the original
terminal and get rid of it when the editor returns, if the terminal
supports erasing the last line.  Also, make sure that our message is
terminated with a whitespace so that any message the editor may show
upon starting up will be kept separate from our message.

Power users might not want to see this message or their editor might
already print such a message (e.g. emacsclient). Allow these users to
suppress the message by disabling the "advice.waitingForEditor" config.

The standard advise() function is not used here as it would always add
a newline which would make deleting the message harder.

Helped-by: Junio C Hamano 
Signed-off-by: Lars Schneider 
Signed-off-by: Junio C Hamano 
---
 Documentation/config.txt |  3 +++
 advice.c |  2 ++
 advice.h |  1 +
 editor.c | 24 
 4 files changed, 30 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9593bfabaa..6ebc50eea8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -351,6 +351,9 @@ advice.*::
addEmbeddedRepo::
Advice on what to do when you've accidentally added one
git repo inside of another.
+   waitingForEditor::
+   Print a message to the terminal whenever Git is waiting for
+   editor input from the user.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index d81e1cb742..af29d23e43 100644
--- a/advice.c
+++ b/advice.c
@@ -17,6 +17,7 @@ int advice_set_upstream_failure = 1;
 int advice_object_name_warning = 1;
 int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
+int advice_waiting_for_editor = 1;
 
 static struct {
const char *name;
@@ -38,6 +39,7 @@ static struct {
{ "objectnamewarning", _object_name_warning },
{ "rmhints", _rm_hints },
{ "addembeddedrepo", _add_embedded_repo },
+   { "waitingforeditor", _waiting_for_editor },
 
/* make this an alias for backward compatibility */
{ "pushnonfastforward", _push_update_rejected }
diff --git a/advice.h b/advice.h
index c84a44531c..f7cbbd342f 100644
--- a/advice.h
+++ b/advice.h
@@ -19,6 +19,7 @@ extern int advice_set_upstream_failure;
 extern int advice_object_name_warning;
 extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
+extern int advice_waiting_for_editor;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/editor.c b/editor.c
index c65ea698eb..8acce0dcd4 100644
--- a/editor.c
+++ b/editor.c
@@ -45,6 +45,23 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
const char *args[] = { editor, real_path(path), NULL };
struct child_process p = CHILD_PROCESS_INIT;
int ret, sig;
+   int print_waiting_for_editor = advice_waiting_for_editor && 
isatty(2);
+
+   if (print_waiting_for_editor) {
+   /*
+* A dumb terminal cannot erase the line later on. Add a
+* newline to separate the hint from subsequent output.
+*
+* In case the editor emits further cruft after what
+* we wrote above, separate it from our message with SP.
+*/
+   const char term = is_terminal_dumb() ? '\n' : ' ';
+
+   fprintf(stderr,
+   _("hint: Waiting for your editor to close the 
file...%c"),
+   term);
+   fflush(stderr);
+   }
 
p.argv = args;
p.env = env;
@@ -63,6 +80,13 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
if (ret)
return error("There was a problem with the editor 
'%s'.",
editor);
+
+   if (print_waiting_for_editor && !is_terminal_dumb())
+   /*
+   

Re: [PATCH v5 2/2] launch_editor(): indicate that Git waits for user input

[Lars Schneider]

> On 07 Dec 2017, at 16:48, Lars Schneider  wrote:
> 
> 
>> On 07 Dec 2017, at 16:43, Junio C Hamano  wrote:
>> 
>> lars.schnei...@autodesk.com writes:
>> ...
> 
> How about this?
> 
>   fprintf(stderr,
>   _("hint: Waiting for your editor to close the 
> file..."));
>   if (is_terminal_dumb())
>   /*
>* A dumb terminal cannot erase the line later 
> on. Add a
>* newline to separate the hint from subsequent 
> output.
>*/
>   fprintf(stderr, "\n")
>   else
>   fprintf(stderr, " ")
> 

I forgot the ";" ... switching between programming languages ;-)

if (is_terminal_dumb())
/*
 * A dumb terminal cannot erase the line later 
on. Add a
 * newline to separate the hint from subsequent 
output.
 */
fprintf(stderr, "\n");
else
fprintf(stderr, " ");



> Can you squash that if you like it?
> 
> Thanks,
> Lars

Re: [WIP 04/15] upload-pack: convert to a builtin

[Johannes Schindelin]

Hi Junio,

On Wed, 6 Dec 2017, Junio C Hamano wrote:

> Brandon Williams  writes:
> 
> > In order to allow for code sharing with the server-side of fetch in
> > protocol-v2 convert upload-pack to be a builtin.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> 
> This looks obvious and straight-forward to a cursory look.
> 
> I vaguely recalled and feared that we on purpose kept this program
> separate from the rest of the system for a reason, but my mailing
> list search is coming up empty.

I only recall that we kept it in the bin/ directory (as opposed to
mlibexec/git-core/) to help with fetching via SSH.

Ciao,
Dscho

Re: [PATCH v5 2/2] launch_editor(): indicate that Git waits for user input

[Junio C Hamano]

lars.schnei...@autodesk.com writes:

> + if (print_waiting_for_editor) {
> + fprintf(stderr,
> + _("hint: Waiting for your editor to close the 
> file... "));
> + if (is_terminal_dumb())
> + /*
> +  * A dumb terminal cannot erase the line later 
> on. Add a
> +  * newline to separate the hint from subsequent 
> output.
> +  */
> + fprintf(stderr, "\n");
> + fflush(stderr);
> + }

Was the trailing whitespace at the end of the hint message intended?

If we expect the editor to spit out additional garbage on the line,
it would probably help to have that SP, but if that is why we have it
there, it probably should be done only when !is_terminal_dumb().

If the trailing SP is merely there by accident, then removal without
changing anything else is also OK.

I cannot tell which is the case, hence this comment.

Thanks.

Re: partial_clone_get_default_filter_spec has no callers

[Jeff Hostetler]

On 12/6/2017 8:59 PM, Ramsay Jones wrote:



On 06/12/17 21:07, Jeff Hostetler wrote:



On 12/6/2017 12:39 PM, Ramsay Jones wrote:

Hi Jeff,

commit f1862e8153 ("partial-clone: define partial clone settings
in config", 2017-12-05), which is part of your 'jh/partial-clone'
branch, introduces the partial_clone_get_default_filter_spec()
function without any callers. Could you please confirm that this
is intentional and that, presumably, a future series will include
a call to this function.


I'll double check.  Thanks.

BTW is there another tool that you're using to find these?
I know I ran make DEVELOPER=1 and make sparse on everything
and didn't see that come up.


In addition to sparse (which finds some of these), I also run a perl
script over the object files after a given build. (The script was
posted to the list by Junio, many moons ago, and I have made several
changes to my local copy).

I am attaching a copy of the script (static-check.pl). Note that the
'stop list' in the script (%def_ok) is _way_ out of date. However, the
way I use the script, that does not matter; I run the script over the
master->next->pu branches and (ignoring the master branch) diff the
result files from branch to branch. For example, tonight I have:

   $ wc -l sc nsc psc
 74 sc
 73 nsc
 75 psc
222 total
   $
   $ diff sc nsc
   44d43
   < oidmap.o- oidmap_remove
   $
   $ diff nsc psc
   43a44
   > list-objects-filter-options.o   - partial_clone_get_default_filter_spec
   58a60
   > sequencer.o - sign_off_header
   $

You also have to be careful with leaving stale object files
laying around from previous builds ('make clean' sometimes
doesn't). Actually, it may be simpler to read a previous mailing
list thread on exactly this subject [1].


[...]


ATB,
Ramsay Jones

[1] 
https://public-inbox.org/git/%3cb21c8a92-4dd5-56d6-ec6a-5709028ea...@ramsayjones.plus.com%3E/



thanks!  maybe you could post something (in contrib/ perhaps)
that would run your script on a pair of commits like t/perf/run.sh.
just a thought.

Jeff

Re: Unfortunate interaction between git diff-index and fakeroot

[Junio C Hamano]

Elazar Leibovich  writes:

> We noticed some unexpected behavior of git, when running git commands under
> fakeroot, and then running another command without fakeroot.
>
> When running, e.g., git status, or git describe --dirty, git can
> update the index file.

Correct.  fakeroot would report that the files that are actually
owned by the user who is running fakeroot are owned by root; the
cached stat information in the index would be "corrected" to say
that they are owned by root.  So once the index is refreshed like
so, things will become consistent.

> The unexpected result is:
>
> "fakeroot git status" updates the index, and the index now says all files
> are owned by uid:0.

You should learn to expect it; that is how fakeroot works.

> "git diff-index --name-only HEAD" is used to test if the git tree is dirty
> without fakeroot, concluding all files have changed, since their owner UID
> is changed.

The lower-level plumbing diff-* commands are meant to be used by
scripts that refresh the index once upfront.

If you are using "diff-index" for the "is the tree dirty?" check
without running "update-index --refresh", then you are not using
the command correctly.

Re: [PATCH] doc: clarify triangular workflow

[Junio C Hamano]

Matthieu Moy  writes:

> Not a native speaker, but according to wikipedia
> (https://en.wikipedia.org/wiki/Singular_they) it's OK to write
> "maintainer [singular, but already neulral] may get merge conflicts when
> they [sinugular they] ..."

Yes.

Re: [PATCH v2] diff: fix --relative without arguments

[Junio C Hamano]

Jacob Keller  writes:

> From: Jacob Keller 
>
> Recently, commit f7923a5ece00 ("diff: use skip_to_optional_val()",
> 2017-12-04) changed how we parsed some diff options, preferring
> skip_to_optional_val instead of a more complex skip_prefix() setup.

I'd expect a moral equivalent of this squashed in when Christian
rerolls the skip-to-optional-val series (with helped-by: attribution
to you, probably).  It does not make much sense to leave the initial
breakage like this one in the history.

Thanks.

Re: git commit file completion recently broke

[Junio C Hamano]

Christian Couder  writes:

>> I do think it may make sense for
>> the "short" one to use NULL, like:
>>
>>   skip_to_optional_val(arg, "--relative, )
>>
>> but maybe some other callers would be more inconvenienced (they may have
>> to current NULL back into the empty string if they want to string
>> "--foo" the same as "--foo=").
>
> I discussed that with Junio and yeah there are many callers that want
> "--foo" to be the same as "--foo=".

Yup, the original thread has details and me saying that assuming all
of them want --foo and --foo= the same is questionable.  The likely
fix would be to use the _default variant with NULL, which was added
exactly for cases like this.

[PATCH v5 0/2] launch_editor(): indicate that Git waits for user input

[lars . schneider]

From: Lars Schneider 

Hi,

Patch 1/2: No change. The patch got "looks good to me" from Peff [1]

Patch 2/2: I changed the waiting message to our bikeshedding result [2] and
   I enabled the waiting message on dumb terminals for consistency.

I also tested the patch on OS X and Windows with smart and dumb terminals.

Thanks for the reviews,
Lars


[1] https://public-inbox.org/git/20171130203042.gb3...@sigill.intra.peff.net/
[2] https://public-inbox.org/git/20171204220908.ga8...@sigill.intra.peff.net/

RFC: 
https://public-inbox.org/git/274b4850-2eb7-4bfa-a42c-25a573254...@gmail.com/
 v1: https://public-inbox.org/git/xmqqr2syvjxb@gitster.mtv.corp.google.com/
 v2: 
https://public-inbox.org/git/20171117135109.18071-1-lars.schnei...@autodesk.com/
 v3: 
https://public-inbox.org/git/20171127134716.69471-1-lars.schnei...@autodesk.com/
 v4: 
https://public-inbox.org/git/20171129143752.60553-1-lars.schnei...@autodesk.com/


Base Ref: master
Web-Diff: https://github.com/larsxschneider/git/commit/c00d4de8cf
Checkout: git fetch https://github.com/larsxschneider/git editor-v5 && git 
checkout c00d4de8cf


### Interdiff (v4..v5):

diff --git a/editor.c b/editor.c
index cdad4f74ec..d52017363c 100644
--- a/editor.c
+++ b/editor.c
@@ -45,11 +45,17 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
const char *args[] = { editor, real_path(path), NULL };
struct child_process p = CHILD_PROCESS_INIT;
int ret, sig;
-   int print_waiting_for_editor = advice_waiting_for_editor &&
-   isatty(2) && !is_terminal_dumb();
+   int print_waiting_for_editor = advice_waiting_for_editor && 
isatty(2);

if (print_waiting_for_editor) {
-   fprintf(stderr, _("hint: Waiting for your editor 
input..."));
+   fprintf(stderr,
+   _("hint: Waiting for your editor to close the 
file... "));
+   if (is_terminal_dumb())
+   /*
+* A dumb terminal cannot erase the line later 
on. Add a
+* newline to separate the hint from subsequent 
output.
+*/
+   fprintf(stderr, "\n");
fflush(stderr);
}

@@ -71,11 +77,10 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
return error("There was a problem with the editor 
'%s'.",
editor);

-   if (print_waiting_for_editor)
+   if (print_waiting_for_editor && !is_terminal_dumb())
/*
-* go back to the beginning and erase the
-* entire line to avoid wasting the vertical
-* space.
+* Go back to the beginning and erase the entire line to
+* avoid wasting the vertical space.
 */
fputs("\r\033[K", stderr);
}


### Patches

Lars Schneider (2):
  refactor "dumb" terminal determination
  launch_editor(): indicate that Git waits for user input

 Documentation/config.txt |  3 +++
 advice.c |  2 ++
 advice.h |  1 +
 cache.h  |  1 +
 color.c  |  3 +--
 editor.c | 29 +++--
 sideband.c   |  5 ++---
 7 files changed, 37 insertions(+), 7 deletions(-)


base-commit: 89ea799ffcc5c8a0547d3c9075eb979256ee95b8
--
2.15.1

[PATCH v5 1/2] refactor "dumb" terminal determination

[lars . schneider]

From: Lars Schneider 

Move the code to detect "dumb" terminals into a single location. This
avoids duplicating the terminal detection code yet again in a subsequent
commit.

Signed-off-by: Lars Schneider 
---
 cache.h| 1 +
 color.c| 3 +--
 editor.c   | 9 +++--
 sideband.c | 5 ++---
 4 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index 89f5d24579..3842fc097c 100644
--- a/cache.h
+++ b/cache.h
@@ -1469,6 +1469,7 @@ extern const char *ident_default_name(void);
 extern const char *ident_default_email(void);
 extern const char *git_editor(void);
 extern const char *git_pager(int stdout_is_tty);
+extern int is_terminal_dumb(void);
 extern int git_ident_config(const char *, const char *, void *);
 extern void reset_ident_date(void);
 
diff --git a/color.c b/color.c
index 9a9261ac16..d48dd947c9 100644
--- a/color.c
+++ b/color.c
@@ -329,8 +329,7 @@ static int check_auto_color(void)
if (color_stdout_is_tty < 0)
color_stdout_is_tty = isatty(1);
if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
-   char *term = getenv("TERM");
-   if (term && strcmp(term, "dumb"))
+   if (!is_terminal_dumb())
return 1;
}
return 0;
diff --git a/editor.c b/editor.c
index 7519edecdc..c65ea698eb 100644
--- a/editor.c
+++ b/editor.c
@@ -7,11 +7,16 @@
 #define DEFAULT_EDITOR "vi"
 #endif
 
+int is_terminal_dumb(void)
+{
+   const char *terminal = getenv("TERM");
+   return !terminal || !strcmp(terminal, "dumb");
+}
+
 const char *git_editor(void)
 {
const char *editor = getenv("GIT_EDITOR");
-   const char *terminal = getenv("TERM");
-   int terminal_is_dumb = !terminal || !strcmp(terminal, "dumb");
+   int terminal_is_dumb = is_terminal_dumb();
 
if (!editor && editor_program)
editor = editor_program;
diff --git a/sideband.c b/sideband.c
index 1e4d684d6c..6d7f943e43 100644
--- a/sideband.c
+++ b/sideband.c
@@ -20,13 +20,12 @@
 
 int recv_sideband(const char *me, int in_stream, int out)
 {
-   const char *term, *suffix;
+   const char *suffix;
char buf[LARGE_PACKET_MAX + 1];
struct strbuf outbuf = STRBUF_INIT;
int retval = 0;
 
-   term = getenv("TERM");
-   if (isatty(2) && term && strcmp(term, "dumb"))
+   if (isatty(2) && !is_terminal_dumb())
suffix = ANSI_SUFFIX;
else
suffix = DUMB_SUFFIX;
-- 
2.15.1

[PATCH v5 2/2] launch_editor(): indicate that Git waits for user input

[lars . schneider]

From: Lars Schneider 

When a graphical GIT_EDITOR is spawned by a Git command that opens
and waits for user input (e.g. "git rebase -i"), then the editor window
might be obscured by other windows. The user might be left staring at
the original Git terminal window without even realizing that s/he needs
to interact with another window before Git can proceed. To this user Git
appears hanging.

Print a message that Git is waiting for editor input in the original
terminal and get rid of it when the editor returns (if the terminal
supports erasing the last line).

Power users might not want to see this message or their editor might
already print such a message (e.g. emacsclient). Allow these users to
suppress the message by disabling the "advice.waitingForEditor" config.

The standard advise() function is not used here as it would always add
a newline which would make deleting the message harder.

Helped-by: Junio C Hamano 
Signed-off-by: Lars Schneider 
---
 Documentation/config.txt |  3 +++
 advice.c |  2 ++
 advice.h |  1 +
 editor.c | 20 
 4 files changed, 26 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 671fcbaa0f..de64201e82 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -354,6 +354,9 @@ advice.*::
ignoredHook::
Advice shown if an hook is ignored because the hook is not
set as executable.
+   waitingForEditor::
+   Print a message to the terminal whenever Git is waiting for
+   editor input from the user.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index c6169bcb52..406efc183b 100644
--- a/advice.c
+++ b/advice.c
@@ -18,6 +18,7 @@ int advice_object_name_warning = 1;
 int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
 int advice_ignored_hook = 1;
+int advice_waiting_for_editor = 1;
 
 static struct {
const char *name;
@@ -40,6 +41,7 @@ static struct {
{ "rmhints", _rm_hints },
{ "addembeddedrepo", _add_embedded_repo },
{ "ignoredhook", _ignored_hook },
+   { "waitingforeditor", _waiting_for_editor },
 
/* make this an alias for backward compatibility */
{ "pushnonfastforward", _push_update_rejected }
diff --git a/advice.h b/advice.h
index f525d6f89c..70568fa792 100644
--- a/advice.h
+++ b/advice.h
@@ -20,6 +20,7 @@ extern int advice_object_name_warning;
 extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
 extern int advice_ignored_hook;
+extern int advice_waiting_for_editor;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/editor.c b/editor.c
index c65ea698eb..d52017363c 100644
--- a/editor.c
+++ b/editor.c
@@ -45,6 +45,19 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
const char *args[] = { editor, real_path(path), NULL };
struct child_process p = CHILD_PROCESS_INIT;
int ret, sig;
+   int print_waiting_for_editor = advice_waiting_for_editor && 
isatty(2);
+
+   if (print_waiting_for_editor) {
+   fprintf(stderr,
+   _("hint: Waiting for your editor to close the 
file... "));
+   if (is_terminal_dumb())
+   /*
+* A dumb terminal cannot erase the line later 
on. Add a
+* newline to separate the hint from subsequent 
output.
+*/
+   fprintf(stderr, "\n");
+   fflush(stderr);
+   }
 
p.argv = args;
p.env = env;
@@ -63,6 +76,13 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
if (ret)
return error("There was a problem with the editor 
'%s'.",
editor);
+
+   if (print_waiting_for_editor && !is_terminal_dumb())
+   /*
+* Go back to the beginning and erase the entire line to
+* avoid wasting the vertical space.
+*/
+   fputs("\r\033[K", stderr);
}
 
if (!buffer)
-- 
2.15.1

Re: [PATCH v4] send-email: extract email-parsing code into a subroutine

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 07 2017, Matthieu Moy jotted:

> Not terribly important, but your patch has trailing newlines. "git diff
> --staged --check" to see them. More below.
>
> PAYRE NATHAN p1508475  writes:
>
>> the part of code which parses the header a last time to prepare the
>> email and send it.
>
> The important point is not that it's the last time the code parses
> headers, so I'd drop the "a last time".
>
>> +my %parsed_email;
>> +$parsed_email{'body'} = '';
>> +while (my $line = <$c>) {
>> +next if $line =~ m/^GIT:/;
>> +parse_header_line($line, \%parsed_email);
>> +if ($line =~ /^\n$/i) {
>
> You don't need the /i (case-Insensitive) here, there are no letters to
> match.

Good catch, actually this can just be: /^$/. The $ syntax already
matches the ending newline, no need for /^\n$/.

>> +sub parse_header_line {
>> +my $lines = shift;
>> +my $parsed_line = shift;
>> +my $pattern1 = join "|", qw(To Cc Bcc);
>> +my $pattern2 = join "|",
>> +qw(From Subject Date In-Reply-To Message-ID MIME-Version
>> +Content-Type Content-Transfer-Encoding References);
>> +
>> +foreach (split(/\n/, $lines)) {
>> +if (/^($pattern1):\s*(.+)$/i) {
>> +$parsed_line->{lc $1} = [ parse_address_line($2) ];
>> +} elsif (/^($pattern2):\s*(.+)\s*$/i) {
>> +$parsed_line->{lc $1} = $2;
>> +}
>
> I don't think you need to list the possibilities in the "else" branch.
> Just matching /^([^:]*):\s*(.+)\s*$/i should do the trick.

Although you'll end up with a lot of stuff in the $parsed_line hash you
don't need, which makes dumping it for debugging verbose.

I also wonder about multi-line headers, but then again that probably
breaks already on e.g. Message-ID and Refererences, but that's an
existing bug unrelated to this patch...

>> +$body = $body . $body_line;
>
> Or just: $body .= $body_line;

Re: [PATCH v4] send-email: extract email-parsing code into a subroutine

[Matthieu Moy]

Not terribly important, but your patch has trailing newlines. "git diff
--staged --check" to see them. More below.

PAYRE NATHAN p1508475  writes:

> the part of code which parses the header a last time to prepare the
> email and send it.

The important point is not that it's the last time the code parses
headers, so I'd drop the "a last time".

> + my %parsed_email;
> + $parsed_email{'body'} = '';
> + while (my $line = <$c>) {
> + next if $line =~ m/^GIT:/;
> + parse_header_line($line, \%parsed_email);
> + if ($line =~ /^\n$/i) {

You don't need the /i (case-Insensitive) here, there are no letters to
match.

> + if ($parsed_email{'mime-version'}) {
> + $need_8bit_cte = 0;

This $need_8bit_cte is a leftover of the old code, which processed the
headers in the order it found them in the message and had to remember
the content of MIME-Version while parsing Content-Type.

I believe you can apply this on top of your patch:

--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -709,7 +709,6 @@ EOT3
open $c, "<", $compose_filename
or die sprintf(__("Failed to open %s: %s"), $compose_filename, 
$!);
 
-   my $need_8bit_cte = file_has_nonascii($compose_filename);
my $in_body = 0;
my $summary_empty = 1;
if (!defined $compose_encoding) {
@@ -740,12 +739,10 @@ EOT3
"\n";
}
if ($parsed_email{'mime-version'}) {
-   $need_8bit_cte = 0;
print $c2 "MIME-Version: $parsed_email{'mime-version'}\n",
"Content-Type: 
$parsed_email{'content-type'};\n",
"Content-Transfer-Encoding: 
$parsed_email{'content-transfer-encoding'}\n";
-   }
-   if ($need_8bit_cte) {
+   } else if (file_has_nonascii($compose_filename)) {
if ($parsed_email{'content-type'}) {
print $c2 "MIME-Version: 1.0\n",
 "Content-Type: 
$parsed_email{'content-type'};",

It reads much better: "If the original message already had a
MIME-Version header, then use that, else see if the file has non-ascii
characters and if so, use MIME-Version: 1.0".

Actually, you can even simplify further by factoring the if/else below:

> + if ($parsed_email{'content-type'}) {
> + print $c2 "MIME-Version: 1.0\n",
> +  "Content-Type: 
> $parsed_email{'content-type'};",

(Suspicious ";", and suspicious absence of "\n" here, I don't think it's
intentional and I'm fixing it below, but correct me if I'm wrong)

> +  "Content-Transfer-Encoding: 8bit\n";
> + } else {

(Broken indentation, this is not aligned with the "if" above)

>   print $c2 "MIME-Version: 1.0\n",
>"Content-Type: text/plain; ",
> -"charset=$compose_encoding\n",
> +  "charset=$compose_encoding\n",
>"Content-Transfer-Encoding: 8bit\n";
>   }

This could become stg like (untested):

} else if (file_has_nonascii($compose_filename)) {
my $content_type = ($parsed_email{'content-type'} or
"text/plain; charset=$compose_encoding");
print $c2 "MIME-Version: 1.0\n",
  "Content-Type: $content_type\n",
  "Content-Transfer-Encoding: 8bit\n";
}

> + open $c2, "<", $compose_filename . ".final"
> + or die sprintf(__("Failed to open %s.final: %s"), 
> $compose_filename, $!);
> + close $c2;

What is this? Cut-and-paste mistake?

> +sub parse_header_line {
> + my $lines = shift;
> + my $parsed_line = shift;
> + my $pattern1 = join "|", qw(To Cc Bcc);
> + my $pattern2 = join "|",
> + qw(From Subject Date In-Reply-To Message-ID MIME-Version 
> + Content-Type Content-Transfer-Encoding References);
> + 
> + foreach (split(/\n/, $lines)) {
> + if (/^($pattern1):\s*(.+)$/i) {
> + $parsed_line->{lc $1} = [ parse_address_line($2) ];
> + } elsif (/^($pattern2):\s*(.+)\s*$/i) {
> + $parsed_line->{lc $1} = $2;
> + }

I don't think you need to list the possibilities in the "else" branch.
Just matching /^([^:]*):\s*(.+)\s*$/i should do the trick.

> + $body = $body . $body_line;

Or just: $body .= $body_line;

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [PATCH] doc: clarify triangular workflow

[Matthieu Moy]

BENSOUSSAN--BOHM DANIEL p1507430
 writes:

>>The document starts with
>
>   >This document attempts to write down and motivate some of the
>   >workflow elements used for `git.git` itself.  Many ideas apply
>   >in general, though the full workflow is rarely required for
>   >smaller projects with fewer people involved.
>
>>and makes me wonder (note: I am not involved in writing any of the
>>existing text in this document) how much material foreign to the
>>actual workflow used for `git.git` should go in here.  Having
>>multiple maintainers at the same time is not a workflow element that
>>we have ever used, for example, so I am not sure about the change in
>>the above paragraph.
>
> We were told to change 'he' into 'they' to be neutral.  However, it's true
> that there's one maintainer at a time so we will remove the 's' from
> "maintainers". 

Not a native speaker, but according to wikipedia
(https://en.wikipedia.org/wiki/Singular_they) it's OK to write
"maintainer [singular, but already neulral] may get merge conflicts when
they [sinugular they] ..."

-- 
Matthieu Moy
https://matthieu-moy.fr/

[PATCH v4] send-email: extract email-parsing code into a subroutine

[Nathan Payre]

The existing code mixes parsing of email header with regular
expression and actual code. Extract the parsing code into a new
subroutine "parse_header_line()". This improves the code readability
and make parse_header_line reusable in other place.

"parsed_header_line()" and "filter_body()" could be used for refactoring
the part of code which parses the header a last time to prepare the
email and send it.

Signed-off-by: Nathan Payre 
Signed-off-by: Matthieu Moy 
Signed-off-by: Timothee Albertin 
Signed-off-by: Daniel Bensoussan 
Signed-off-by: Junio C Hamano 
Thanks-to: Ævar Arnfjörð Bjarmason 
---

This patch fixes the indentation problem and reduce lines over 80 characters.

 git-send-email.perl | 102 ++--
 1 file changed, 75 insertions(+), 27 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2208dcc21..b64f8872d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -715,41 +715,60 @@ EOT3
if (!defined $compose_encoding) {
$compose_encoding = "UTF-8";
}
-   while(<$c>) {
-   next if m/^GIT:/;
-   if ($in_body) {
-   $summary_empty = 0 unless (/^\n$/);
-   } elsif (/^\n$/) {
-   $in_body = 1;
-   if ($need_8bit_cte) {
+
+
+   my %parsed_email;
+   $parsed_email{'body'} = '';
+   while (my $line = <$c>) {
+   next if $line =~ m/^GIT:/;
+   parse_header_line($line, \%parsed_email);
+   if ($line =~ /^\n$/i) {
+   $parsed_email{'body'} = filter_body($c);
+   }
+   }
+
+   if ($parsed_email{'from'}) {
+   $sender = $parsed_email{'from'};
+   }
+   if ($parsed_email{'in-reply-to'}) {
+   $initial_reply_to = $parsed_email{'in-reply-to'};
+   }
+   if ($parsed_email{'subject'}) {
+   $initial_subject = $parsed_email{'subject'};
+   print $c2 "Subject: " .
+   quote_subject($parsed_email{'subject'}, 
$compose_encoding) .
+   "\n";
+   }
+   if ($parsed_email{'mime-version'}) {
+   $need_8bit_cte = 0;
+   print $c2 "MIME-Version: $parsed_email{'mime-version'}\n",
+   "Content-Type: 
$parsed_email{'content-type'};\n",
+   "Content-Transfer-Encoding: 
$parsed_email{'content-transfer-encoding'}\n";
+   }
+   if ($need_8bit_cte) {
+   if ($parsed_email{'content-type'}) {
+   print $c2 "MIME-Version: 1.0\n",
+"Content-Type: 
$parsed_email{'content-type'};",
+"Content-Transfer-Encoding: 8bit\n";
+   } else {
print $c2 "MIME-Version: 1.0\n",
 "Content-Type: text/plain; ",
-  "charset=$compose_encoding\n",
+"charset=$compose_encoding\n",
 "Content-Transfer-Encoding: 8bit\n";
}
-   } elsif (/^MIME-Version:/i) {
-   $need_8bit_cte = 0;
-   } elsif (/^Subject:\s*(.+)\s*$/i) {
-   $initial_subject = $1;
-   my $subject = $initial_subject;
-   $_ = "Subject: " .
-   quote_subject($subject, $compose_encoding) .
-   "\n";
-   } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
-   $initial_reply_to = $1;
-   next;
-   } elsif (/^From:\s*(.+)\s*$/i) {
-   $sender = $1;
-   next;
-   } elsif (/^(?:To|Cc|Bcc):/i) {
-   print __("To/Cc/Bcc fields are not interpreted yet, 
they have been ignored\n");
-   next;
-   }
-   print $c2 $_;
}
+   if ($parsed_email{'body'}) {
+   $summary_empty = 0;
+   print $c2 "\n$parsed_email{'body'}\n";
+   }
+
close $c;
close $c2;
 
+   open $c2, "<", $compose_filename . ".final"
+   or die sprintf(__("Failed to open %s.final: %s"), 
$compose_filename, $!);
+   close $c2;
+
if ($summary_empty) {
print __("Summary email is empty, skipping it\n");
$compose = -1;
@@ -792,6 +811,35 @@ sub ask {
return;
 }
 
+sub parse_header_line {
+   my $lines = shift;
+   my $parsed_line = shift;
+   my $pattern1 = join "|", qw(To Cc Bcc);
+   my 

Re: 'git worktree add' does not fire post-checkout hook

[Eric Sunshine]

On Wed, Dec 6, 2017 at 4:45 PM, Eric Sunshine  wrote:
> On Wed, Dec 6, 2017 at 4:00 PM, Gumbel, Matthew K
>  wrote:
>> I've noticed that when I run 'git worktree add /path/to/new/tree',
>> the post-checkout hook does not fire, even though the worktree
>> manpage explicitly states that "worktree add" will, "Create 
>> and checkout  into it."
>>
>> Is this the intended behavior? Seems like maybe a bug, but I'm not
>> sure.
>
> If you'd like to get your feet wet at contributing to the Git project,
> this might be a good first dip, as it looks like an easy fix (a one-
> or two-liner). The only thing which needs a bit of care is to skip the
> hook when --no-checkout is specified. Other than that, 'githooks'
> documentation would need an update to mention that git-worktree also
> runs the hook, and t2025-worktree-add.sh would want a couple new tests
> (which would probably be the most complex part of the patch).

I worked up a patch to fix this oversight which I'll try to send out
later today.

re

[Eiman Yousef Al-muzafar]

Assalam Alaikum, I am Eiman Yousef M A Al-muzafar, a Muslim woman from Qatar, I 
am contacting you regarding a relationship of trust and confidence for an 
inheritance. Please contact me on my private email for more details: 
ey_muza...@hotmail.com

Re: partial_clone_get_default_filter_spec has no callers

[Luc Van Oostenryck]

On Thu, Dec 07, 2017 at 01:59:26AM +, Ramsay Jones wrote:
> 
> BTW, if you are using a version of sparse post v0.5.1, you can
> get rid of the only sparse warning on Linux (assuming you don't
> build with NO_REGEX set), by using the -Wno-memcpy-max-count option;
> I have the following set in my config.mak:
> 
>   $ tail -2 config.mak
>   pack-revindex.sp: SPARSE_FLAGS += -Wno-memcpy-max-count
> 
>   $ 
> 
> [I haven't sent a proper patch, since the required version of
> sparse is not widely available yet.]

Note that sparse simply ignore unknown options/flags, so adding
it now won't create a problem and can already help those using
a recent vesion of sparse.

Regards,
-- Luc 

RE: [PATCH] doc: clarify triangular workflow

[BENSOUSSAN--BOHM DANIEL p1507430]

>The document starts with

  >This document attempts to write down and motivate some of the
  >workflow elements used for `git.git` itself.  Many ideas apply
  >in general, though the full workflow is rarely required for
  >smaller projects with fewer people involved.

>and makes me wonder (note: I am not involved in writing any of the
>existing text in this document) how much material foreign to the
>actual workflow used for `git.git` should go in here.  Having
>multiple maintainers at the same time is not a workflow element that
>we have ever used, for example, so I am not sure about the change in
>the above paragraph.

We were told to change 'he' into 'they' to be neutral.  However, it's true
that there's one maintainer at a time so we will remove the 's' from
"maintainers". 

>> +TRIANGULAR WORKFLOW
>> +---

>I really hate to say this.  Before I made comment on the last round
>that tried to add this section, I didn't read the original closely
>enough.

>The thing is, it does already cover the triangular workflow in the
>"Merge workflow" section (you may need to already know what you are
>reading to realize that fact, though).  The text in the existing
>"Merge workflow" section where requestor pushes to somewhere for the
>maintainer to pull from may not be immediately obvious, and it may
>be worthwhile to improve it, but I find it highly misleading to add
>an entirely new section as if it is describing yet another separate
>workflow that is different from anything that is already described
>in the document.  It is not.

>A replacement of the entire section (but I'd recommend keeping the
>"Merge workflow" title, which contrasts well with the other "Patch
>workflow" that follows), or a separate document that is referred to
>with "see that other one for a lengthier description" by the
>existing "Merge workflow" section, or somewhere in between, might be
>a more acceptable organization, though.

We'll take this into account.  We will create a new file called
"triangularworkflow.txt" just for the triangular workflow to be more precise
because "gitworkflows.txt" is a long file.  More, we first wanted to change
the doc to help new contributors. If we put all the triangular workflow
section in merge workflows, this won't be clear for a new contributor.

Thank you for the review.

Daniel BENSOUSSAN-BOHM

Unfortunate interaction between git diff-index and fakeroot

[Elazar Leibovich]

Hi,

We noticed some unexpected behavior of git, when running git commands under
fakeroot, and then running another command without fakeroot.

When running, e.g., git status, or git describe --dirty, git can
update the index file.

This can happen inadvertently by, e.g., running the make install process of
a package under fakeroot, and running "git describe --dirty", or "git
status" to generate package name.

The unexpected result is:

"fakeroot git status" updates the index, and the index now says all files
are owned by uid:0.

"git diff-index --name-only HEAD" is used to test if the git tree is dirty
without fakeroot, concluding all files have changed, since their owner UID
is changed.

You can easily recreate it:

$ mkdir gitexample;cd gitexample;git init
Initialized empty Git repository in /home/elazar/gitexample/.git/
$ touch x; git add x; git commit -m init
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 x
$ git diff-index HEAD # nothing is printed, since working dir clean
$ fakeroot git diff # index is updated to contain uid:0
$ git ls-files -s --debug
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0   x
  ctime: 1512636533:915178157
  mtime: 1512636533:915178157
  dev: 64515ino: 8523907
  uid: 0gid: 0
  size: 0   flags: 0
$ git diff-index --name-only HEAD # x is considered different now
x

Make sure you do not have a shell prompt that calls git status in
between, like oh-my-zsh, or bash-it.

This is problematic because:

1. File owner is not really tracked by git porcelain, and I'm not sure
   what's the benefit of tracking ownership of files in git diff-index. 
Users
   who expect git diff to return "no changes" after ownership changed, 
expect

   git diff-index to return "no changes" as well.

  This is supported by the manual page who say "Compares the content 
and mode
  of the blobs found in a tree object with the corresponding tracked 
files in

  the working tree", and doesn't mention file ownership.

2. Linux tree uses git diff-index to test if the working tree is dirty. It
   can, and has cause subtle issues when building and packaging different
   parts of the tree to different directories concurrently.

Our proposed solutions:

Make match_stat_data ignore ownership changes.

I searched git source for usage of OWNER_CHANGED bit and found that no one
tests for it. I can't think of a scenario where the fact that the owner have
changed would matter.

I searched for usage of git-diff-index in git tree, and found that
git-stash uses it. This is not relevant since stash would ignore ownership
changes. I saw that merge-octupus and filter-branch uses git-diff-index
but they also don't seem to care if ownership has changed.

I searched for the first commit who tracked file ownership in the index,
it was there from the first commit e83c5163316f89bfbde7d9ab23ca2e25604af290
I found no specific reason given for tracking the owner in related commits.

You can see a change in a similar spirit to what I propose in commit
2cb45e95438c113871fbbea5b4f629f9463034e7 where st_dev is ignored by default
due to similar problem in NFS.

What do you think?

Should the behavior remain, and diff-index should say it tracks "mode and
owner changes"?
Would it be OK to change the actual behavior of diff-index to ignore owner
changes?

[PATCH] setup.c: fix comment about order of .git directory discovery

[SZEDER Gábor]

Since gitfiles were introduced in b44ebb19e (Add platform-independent
.git "symlink", 2008-02-20) the order of checks during .git directory
discovery is: gitfile, gitdir, bare repo.  However, that commit did
only partially update the in-code comment describing this order,
missing the last line which still puts gitdir before gitfile.

Signed-off-by: SZEDER Gábor 
---
 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 03f51e056..b168d25db 100644
--- a/setup.c
+++ b/setup.c
@@ -921,7 +921,7 @@ static enum discovery_result 
setup_git_directory_gently_1(struct strbuf *dir,
 * - ../.git
 * - ../.git/
 * - ../ (bare)
-* - ../../.git/
+* - ../../.git
 *   etc.
 */
one_filesystem = !git_env_bool("GIT_DISCOVERY_ACROSS_FILESYSTEM", 0);
-- 
2.15.1.338.g53352fef7

Re: git commit file completion recently broke

[Jeff King]

On Thu, Dec 07, 2017 at 09:14:31AM +0100, Christian Couder wrote:

> > I do think it may make sense for
> > the "short" one to use NULL, like:
> >
> >   skip_to_optional_val(arg, "--relative, )
> >
> > but maybe some other callers would be more inconvenienced (they may have
> > to current NULL back into the empty string if they want to string
> > "--foo" the same as "--foo=").

Oof, I lost all ability to type in that last sentence. :)

It looks like you deciphered my meaning, though.

> I discussed that with Junio and yeah there are many callers that want
> "--foo" to be the same as "--foo=".

Yeah, if this is the only case, then just calling the "_default" variant
with NULL for this instance makes sense to me.

> By the way I wonder if "--relative=" makes any sense, and if we should
> emit a warning or just die in this case.

I also wondered about that. It does function as "relative to the root of
the tree". But of course if you want that you can just omit
"--relative". Still, it could be a minor convenience for somebody who is
filling in "--relative" in a script. That's reaching, I think, but I
don't see any particular reason to _forbid_ it (especially since it has
worked for many years).

-Peff

Re: git commit file completion recently broke

[Christian Couder]

On Thu, Dec 7, 2017 at 1:56 AM, Jeff King  wrote:

> I think we'd do better to just assign NULL when there's "=", so we can
> tell the difference between "--relative", "--relative=", and
> "--relative=foo" (all of which are distinct).
>
> I think that's possible with the current scheme by doing:
>
>   else if (skip_to_optional_val_default(arg, "--relative", , NULL)) {
> options->flags.relative_name = 1;
> if (arg)
> options->prefix = arg;
>   }

Yeah, that is a possible fix.

> IOW, the problem isn't in the design of the skip function, but just how
> it was used in this particular case.

I agree.

> I do think it may make sense for
> the "short" one to use NULL, like:
>
>   skip_to_optional_val(arg, "--relative, )
>
> but maybe some other callers would be more inconvenienced (they may have
> to current NULL back into the empty string if they want to string
> "--foo" the same as "--foo=").

I discussed that with Junio and yeah there are many callers that want
"--foo" to be the same as "--foo=".

By the way I wonder if "--relative=" makes any sense, and if we should
emit a warning or just die in this case.

Re: Assalam Alaikum

[Eiman Yousef M A Al-muzafar]

Assalam Alaikum, I am Eiman Yousef M A Al-muzafar, a Muslim woman from Qatar, I 
am contacting you regarding a relationship of trust and confidence for an 
inheritance. Please contact me on my private email for more details: 
eimanyou...@hotmail.com