date:20181106

Jeff King  writes:

> On Tue, Nov 06, 2018 at 04:48:28PM -0800, Carlo Arenas wrote:
>
> I think date_yesterday() is the only one of those special functions that
> gets called like this. Here's what I think we should do to fix it (this
> can go right on top of jk/misc-unused-fixes, which is already in next).

Thanks, both.  I think the patch makes sense.

> -- >8 --
> Subject: [PATCH] approxidate: fix NULL dereference in date_time()
>
> When we see a time like "noon", we pass "12" to our date_time() helper,
> which sets the hour to 12pm. If the current time is before noon, then we
> wrap around to yesterday using date_yesterday(). But unlike the normal
> calls to date_yesterday() from approxidate_alpha(), we pass a NULL "num"
> parameter. Since c27cc94fad (approxidate: handle pending number for
> "specials", 2018-11-02), that causes a segfault.
>
> One way to fix this is by checking for NULL. But arguably date_time() is
> abusing our helper by passing NULL in the first place (and this is the
> only case where one of these "special" parsers is used this way). So
> instead, let's have it just do the 1-day subtraction itself. It's still
> just a one-liner due to our update_tm() helper.
>
> Note that the test added here is a little funny, as we say "10am noon",
> which makes the "10am" seem pointless.  But this bug can only be
> triggered when it the currently-parsed hour is before the special time.
> The latest special time is "tea" at 1700, but t0006 uses a hard-coded
> TEST_DATE_NOW of 1900. We could reset TEST_DATE_NOW, but that may lead
> to confusion in other tests. Just saying "10am noon" makes this test
> self-contained.
>
> Reported-by: Carlo Arenas 
> Signed-off-by: Jeff King 
> ---
>  date.c  | 2 +-
>  t/t0006-date.sh | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/date.c b/date.c
> index 7adce327a3..9bc15df6f9 100644
> --- a/date.c
> +++ b/date.c
> @@ -929,7 +929,7 @@ static void date_yesterday(struct tm *tm, struct tm *now, 
> int *num)
>  static void date_time(struct tm *tm, struct tm *now, int hour)
>  {
>   if (tm->tm_hour < hour)
> - date_yesterday(tm, now, NULL);
> + update_tm(tm, now, 24*60*60);
>   tm->tm_hour = hour;
>   tm->tm_min = 0;
>   tm->tm_sec = 0;
> diff --git a/t/t0006-date.sh b/t/t0006-date.sh
> index b7ea5fbc36..ffb2975e48 100755
> --- a/t/t0006-date.sh
> +++ b/t/t0006-date.sh
> @@ -114,6 +114,7 @@ check_approxidate '15:00' '2009-08-30 15:00:00'
>  check_approxidate 'noon today' '2009-08-30 12:00:00'
>  check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
>  check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
> +check_approxidate '10am noon' '2009-08-29 12:00:00'
>  
>  check_approxidate 'last tuesday' '2009-08-25 19:20:00'
>  check_approxidate 'July 5th' '2009-07-05 19:20:00'

Re: [PATCH v2 1/1] pack-objects: ignore ambiguous object warnings

Jeff King  writes:

> So we'd never expect to see anything except "1" in our save_warning
> variable. Doing a save/restore is just about code hygiene and
> maintainability.

Here is what I plan to queue.  Thanks, both.

-- >8 --
From: Derrick Stolee 
Date: Tue, 6 Nov 2018 12:34:47 -0800
Subject: [PATCH] pack-objects: ignore ambiguous object warnings

A git push process runs several processes during its run, but one
includes git send-pack which calls git pack-objects and passes
the known have/wants into stdin using object ids. However, the
default setting for core.warnAmbiguousRefs requires git pack-objects
to check for ref names matching the ref_rev_parse_rules array in
refs.c. This means that every object is triggering at least six
"file exists?" queries.  When there are a lot of refs, this can
add up significantly! I observed a simple push spending three
seconds checking these paths.

The fix here is similar to 4c30d50 "rev-list: disable object/refname
ambiguity check with --stdin".  While the get_object_list() method
reads the objects from stdin, turn warn_on_object_refname_ambiguity
flag (which is usually true) to false.  Just for code hygiene, save
away the original at the beginning and restore it once we are done.

Helped-by: Jeff King 
Signed-off-by: Derrick Stolee 
Signed-off-by: Junio C Hamano 
---
 builtin/pack-objects.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d1144a8f7e..f703e6df9b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2988,6 +2988,7 @@ static void get_object_list(int ac, const char **av)
struct rev_info revs;
char line[1000];
int flags = 0;
+   int save_warning;

init_revisions(, NULL);
save_commit_buffer = 0;
@@ -2996,6 +2997,9 @@ static void get_object_list(int ac, const char **av)
/* make sure shallows are read */
is_repository_shallow(the_repository);

+   save_warning = warn_on_object_refname_ambiguity;
+   warn_on_object_refname_ambiguity = 0;
+
while (fgets(line, sizeof(line), stdin) != NULL) {
int len = strlen(line);
if (len && line[len - 1] == '\n')
@@ -3022,6 +3026,8 @@ static void get_object_list(int ac, const char **av)
die(_("bad revision '%s'"), line);
}

+   warn_on_object_refname_ambiguity = save_warning;
+
if (use_bitmap_index && !get_object_list_from_bitmap())
return;

-- 
2.19.1-856-g8858448bb4

Re: [PATCH 1/1] Windows: force-recompile git.res for differing architectures

Johannes Sixt  writes:

> Am 06.11.18 um 15:55 schrieb Johannes Schindelin via GitGitGadget:
>> From: Johannes Schindelin 
>>
>> When git.rc is compiled into git.res, the result is actually dependent
>> on the architecture. That is, you cannot simply link a 32-bit git.res
>> into a 64-bit git.exe.
>>
>> Therefore, to allow 32-bit and 64-bit builds in the same directory, we
>> let git.res depend on GIT-PREFIX so that it gets recompiled when
>> compiling for a different architecture (this works because the exec path
>> changes based on the architecture: /mingw32/libexec/git-core for 32-bit
>> and /mingw64/libexec/git-core for 64-bit).
>
> On Linux, when I recompile for a different architecture, CFLAGS would
> change, so I would have thought that GIT-CFLAGS were the natural
> choice for a dependency. Don't they change in this case on Windows,
> too?

Depending on GIT-CFLAGS would have a better chance of being safe, I
guess, even though it can at times be overly safe, than GIT-PREFIX,
I suspect.  As a single user target in Makefile, which is only used
by Dscho, who intends to stick to /mingw(32|64)/ convention til the
end of time, I think the posted patch is OK, though.

Re: [PATCH v5 10/12] Add a base implementation of SHA-256 support

2018-11-06 Thread brian m. carlson

On Mon, Nov 05, 2018 at 12:39:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
> On Sun, Nov 04 2018, brian m. carlson wrote:
> > +   {
> > +   "sha256",
> > +   /* "s256", big-endian */
> 
> The existing entry/comment for sha1 is:
> 
>   "sha1",
>   /* "sha1", big-endian */
> 
> So why the sha256/s256 difference in the code/comment? Wondering if I'm
> missing something and we're using "s256" for something.

Ah, good question.  The comment refers to the format_id field which
follows this comment.  The value is the big-endian representation of
"s256" as a 32-bit value.  I picked that over "sha2" to avoid confusion
in case we add SHA-512 in the future, since that's also an SHA-2
algorithm.

Config files and command-line interfaces will use "sha1" or "sha256",
and binary formats will use those 32-bit values ("sha1" or "s256").

> >  const char *empty_tree_oid_hex(void)
> > diff --git a/sha256/block/sha256.c b/sha256/block/sha256.c
> > [...]
> 
> I had a question before about whether we see ourselves perma-forking
> this implementation based off libtomcrypt, as I recall you said yes.

Yes.

> Still, I think it would be better to introduce this in at least two-four
> commits where the upstream code is added as-is, then trimmed down to
> size, then adapted to our coding style, and finally we add our own
> utility functions.

At this point, the only code that's actually used from libtomcrypt is
the block transform.  The upstream code is split over multiple files in
multiple directories and won't compile in our codebase without many
files and a lot of work, so I don't feel good about either including
code that doesn't compile or including large numbers of files that don't
meet our coding standards (and that may still not compile because of
platform issues).

> It'll make it easier to forward-port any future upstream changes.

I don't foresee many, if any, changes to this code.  It either
implements the specification or it doesn't, and it's objectively easy to
determine which.  There's not even an argument to port performance
improvements, since almost everyone will be using a crypto library to
provide this code because libraries perform so dramatically better.
I've tried to not make the code perform worse than it did originally,
but that's it.

Furthermore, the modified code carries a relatively small amount of
resemblance to the original, so if we did port changes forward, we'd
probably have conflicts.

It seems like you really want to include the upstream code as a separate
commit and I understand where you're coming from with wanting to have
this split out into logical commits, but due to the specific nature of
this code, I see a lot of downsides and not a lot of upsides.

> > +   perl -E "for (1..10) { print q{aa}; }" | \
> > +   test-tool sha256 >actual &&
> > +   grep cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0 
> > actual &&
> > +   perl -E "for (1..10) { print q{abcdefghijklmnopqrstuvwxyz}; }" | \
> > +   test-tool sha256 >actual &&
> 
> I've been wanting to make use depend on perl >= 5.10 (previous noises
> about that on-list), but for now we claim to support >=5.8, which
> doesn't have the -E switch.

Good point.  I'll fix that.  After having written a lot of one-liners,
I always write -E, and this was originally a one-liner.

> But most importantly you aren't even using -E features here, and this
> isn't very idoimatic Perl. Instead do, respectively:
> 
> perl -e 'print q{aa} x 10'
> perl -e "print q{abcdefghijklmnopqrstuvwxyz} x 10"

I considered the more idiomatic version originally, but the latter could
allocate a decent amount of memory in one chunk, and I wanted to avoid
that.  I think what I'd like to do, actually, is turn on autoflush and
use a postfix for, which would be more idiomatic and could potentially
provide better testing of the chunking code.  I'll add a comment to that
effect.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

signature.asc
Description: PGP signature

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

Ramsay Jones  writes:

> On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin 
>> 
>> On Windows, an absolute POSIX path needs to be turned into a Windows
>> one.
>> 
>> Signed-off-by: Johannes Schindelin 
>> ---
>>  path.c | 5 +
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/path.c b/path.c
>> index 34f0f98349..a72abf0e1f 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -11,6 +11,7 @@
>>  #include "path.h"
>>  #include "packfile.h"
>>  #include "object-store.h"
>> +#include "exec-cmd.h"
>>  
>>  static int get_st_mode_bits(const char *path, int *mode)
>>  {
>> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>>  
>>  if (path == NULL)
>>  goto return_null;
>> +#ifdef __MINGW32__
>> +if (path[0] == '/')
>> +return system_path(path + 1);
>> +#endif
>
> Hmm, this doesn't quite fit with the intended use of this
> function! ;-) (even on windows!)
>
> I haven't looked very deeply, but doesn't this affect all
> absolute paths in the config read by git_config_pathname(),
> along with all 'included config' files?

I think so.  I have not thought things through to say if replacing a
"full path in the current drive" with system_path() is a sensible
thing to do in the first place, but I am getting the impression from
review comments that it probably is not.

> I am pretty sure that I would not want the absolute paths
> in my config file(s) magically 'moved' depending on whether
> git has been compiled with 'runtime prefix' support or not!

In any case, the helper is about expanding ~/foo and ~who/foo to
absolute paths, without touching other paths, so it is a wrong
function to implement it in, even if the motivation were sensible.

Re: [PATCH 2/3] approxidate: handle pending number for "specials"

On Tue, Nov 06, 2018 at 04:48:28PM -0800, Carlo Arenas wrote:

> On Thu, Nov 1, 2018 at 10:24 PM Jeff King  wrote:
> >
> >  static void date_yesterday(struct tm *tm, struct tm *now, int *num)
> >  {
> > +   *num = 0;
> 
> the only caller (date_time) for this sends num = NULL, so this
> triggers a segfault.

Oof. Thanks for catching.

That's not the only caller. Usually this is called via a function
pointer from the "struct special" array. date_time() is just reusing the
other helper as a quick way to do a one-day subtraction.

> the only reference I could find to that apparently unused parameter comes 
> from:
> 93cfa7c7a8 ("approxidate_careful() reports errorneous date string", 
> 2010-01-26)
> and seems to indicate it is optional and therefore a check for NULL
> might make sense for all other cases

I think date_yesterday() is the only one of those special functions that
gets called like this. Here's what I think we should do to fix it (this
can go right on top of jk/misc-unused-fixes, which is already in next).

-- >8 --
Subject: [PATCH] approxidate: fix NULL dereference in date_time()

When we see a time like "noon", we pass "12" to our date_time() helper,
which sets the hour to 12pm. If the current time is before noon, then we
wrap around to yesterday using date_yesterday(). But unlike the normal
calls to date_yesterday() from approxidate_alpha(), we pass a NULL "num"
parameter. Since c27cc94fad (approxidate: handle pending number for
"specials", 2018-11-02), that causes a segfault.

One way to fix this is by checking for NULL. But arguably date_time() is
abusing our helper by passing NULL in the first place (and this is the
only case where one of these "special" parsers is used this way). So
instead, let's have it just do the 1-day subtraction itself. It's still
just a one-liner due to our update_tm() helper.

Note that the test added here is a little funny, as we say "10am noon",
which makes the "10am" seem pointless.  But this bug can only be
triggered when it the currently-parsed hour is before the special time.
The latest special time is "tea" at 1700, but t0006 uses a hard-coded
TEST_DATE_NOW of 1900. We could reset TEST_DATE_NOW, but that may lead
to confusion in other tests. Just saying "10am noon" makes this test
self-contained.

Reported-by: Carlo Arenas 
Signed-off-by: Jeff King 
---
 date.c  | 2 +-
 t/t0006-date.sh | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/date.c b/date.c
index 7adce327a3..9bc15df6f9 100644
--- a/date.c
+++ b/date.c
@@ -929,7 +929,7 @@ static void date_yesterday(struct tm *tm, struct tm *now, 
int *num)
 static void date_time(struct tm *tm, struct tm *now, int hour)
 {
if (tm->tm_hour < hour)
-   date_yesterday(tm, now, NULL);
+   update_tm(tm, now, 24*60*60);
tm->tm_hour = hour;
tm->tm_min = 0;
tm->tm_sec = 0;
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index b7ea5fbc36..ffb2975e48 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -114,6 +114,7 @@ check_approxidate '15:00' '2009-08-30 15:00:00'
 check_approxidate 'noon today' '2009-08-30 12:00:00'
 check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
 check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
+check_approxidate '10am noon' '2009-08-29 12:00:00'

 check_approxidate 'last tuesday' '2009-08-25 19:20:00'
 check_approxidate 'July 5th' '2009-07-05 19:20:00'
-- 
2.19.1.1534.g3beb3b7d96

Re: [PATCH v2] range-diff: add a --no-patch option to show a summary

Ævar Arnfjörð Bjarmason   writes:

> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> index f01a0be851..05d1f6b6b6 100644
> --- a/builtin/range-diff.c
> +++ b/builtin/range-diff.c
> @@ -16,11 +16,14 @@ int cmd_range_diff(int argc, const char **argv, const 
> char *prefix)
>   int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
>   struct diff_options diffopt = { NULL };
>   int simple_color = -1;
> + int no_patch = 0;
>   struct option options[] = {
>   OPT_INTEGER(0, "creation-factor", _factor,
>   N_("Percentage by which creation is weighted")),
>   OPT_BOOL(0, "no-dual-color", _color,
>   N_("use simple diff colors")),
> + OPT_BOOL_F('s', "no-patch", _patch,
> +  N_("show patch output"), PARSE_OPT_NONEG),

As OPT_BOOL("patch") natively takes "--no-patch" to flip the bool
off, an int variable "patch" that is initialized to 1 would make it
more readable by avoiding double negation !no_patch like the one we
see below.  I guess the reason behind the contortion you wanted to
give the synonym --silent to it?

> @@ -92,7 +95,7 @@ int cmd_range_diff(int argc, const char **argv, const char 
> *prefix)
>   }
>  
>   res = show_range_diff(range1.buf, range2.buf, creation_factor,
> -   simple_color < 1, );
> +   simple_color < 1, !no_patch, );

>   strbuf_release();
>   strbuf_release();

> @@ -7,6 +7,7 @@
>  
>  int show_range_diff(const char *range1, const char *range2,
>   int creation_factor, int dual_color,
> + int patch,
>   struct diff_options *diffopt);

Other than that small "Huh?", the code looks good to me.

> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 6aae364171..27e071650b 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -122,6 +122,26 @@ test_expect_success 'changed commit' '
>   test_cmp expected actual
>  '
>  
> +test_expect_success 'changed commit -p & --patch' '
> + git range-diff --no-color -p topic...changed >actual &&
> + test_cmp expected actual &&
> + git range-diff --no-color --patch topic...changed >actual &&
> + test_cmp expected actual

This makes sure that -p and --patch produces the same output as the
default case?  I am not sure who in the parseopt API groks the
single letter "-p" in this case offhand.  Care to explain how?

The other side of the test to see -s/--no-patch we see below also
makes sense.

> +test_expect_success 'changed commit -s & --no-patch' '
> +...
> + cat >expected <<-EOF &&

Quote EOF to allow readers skim the contents without looking for and
worrying about $substitutions in there, unless there are tons of
offending code in the same script already in which case we should
leave the clean-up outside this primary change.

Re: [PATCH 2/3] approxidate: handle pending number for "specials"

2018-11-06 Thread Carlo Arenas

On Thu, Nov 1, 2018 at 10:24 PM Jeff King  wrote:
>
>  static void date_yesterday(struct tm *tm, struct tm *now, int *num)
>  {
> +   *num = 0;

the only caller (date_time) for this sends num = NULL, so this
triggers a segfault.

the only reference I could find to that apparently unused parameter comes from:
93cfa7c7a8 ("approxidate_careful() reports errorneous date string", 2010-01-26)
and seems to indicate it is optional and therefore a check for NULL
might make sense for all other cases

Carlo

Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert

Jeff King  writes:

> Could we help the reading scripts by normalizing old and new output via
> interpret-trailers, %(trailers), etc?
>
> I think "(cherry picked from ...)" is already considered a trailer by
> the trailer code.

;-)  

Great minds think alike, I guess.  I think it is a great idea
to ask interpret-trailers to help us here.

Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert

Nguyễn Thái Ngọc Duy   writes:

> OK here is a less constroversal attempt to add new trailers. Instead
> of changing the default behavior (which could be done incrementally
> later), this patch simply adds a new option --append-trailer to revert
> and cherry-pick.

I almost agree, except that the the textual description in a revert
and the funny-looking trailer-wannabe in a cherry-pick are two
fundamentally different things, and adding duplicate to the latter
does not make much sense, while keeping both does make sense for the
former.

> PS. maybe --append-trailer is too generic? We have --signoff which is
> also a trailer. But that one carries more weights than just "machine
> generated tags".

> + if (opts->append_trailer) {
> + strbuf_addstr(, "\n");
> + if (parent_id != -1)
> + strbuf_addf(, "Reverts: %s~%d\n",
> + oid_to_hex(>object.oid),
> + parent_id);
> + else
> + strbuf_addf(, "Reverts: %s\n",
> + oid_to_hex(>object.oid));
> + }

Makes sense, I guess.

> @@ -1780,14 +1792,28 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   if (find_commit_subject(msg.message, ))
>   strbuf_addstr(, p);
>  
> - if (opts->record_origin) {
> + if (opts->record_origin || opts->append_trailer) {
>   strbuf_complete_line();
>   if (!has_conforming_footer(, NULL, 0))
>   strbuf_addch(, '\n');
> + }
> +
> + if (opts->record_origin) {
>   strbuf_addstr(, cherry_picked_prefix);
>   strbuf_addstr(, oid_to_hex(>object.oid));
>   strbuf_addstr(, ")\n");
>   }
> + if (opts->append_trailer) {

These should be either-or, I would think, as adding exactly the same
piece of information in two different machine-readable form does not
make much sense.  The command line parser may even want to make sure
that both are not given at the same time.

Alternatively, we can keep using -x as "we are going to record where
the change was cherry-picked from" and use .record_origin to store
that bit, and use the new .append_trailer bit as "if we are recording
the origin, in which format between the old and the new do we do
so?" bit.

I think the latter makes more sense, at least to me.

> + if (opts->record_origin)
> + strbuf_addstr(, "\n");
> + if (parent_id != -1)
> + strbuf_addf(, "Cherry-picked-from: 
> %s~%d\n",
> + oid_to_hex(>object.oid),
> + parent_id);
> + else
> + strbuf_addf(, "Cherry-picked-from: %s\n",
> + oid_to_hex(>object.oid));
> + }

Re: What exactly is a "initial checkout"

Christian Halstrick  writes:

> I am trying to teach JGit [1] to behave like native git regarding some
> corner cases during "git checkout". I am reading the "git read-tree"
> documentation and I am not sure about the case [2]. Git should behave
> differently during a normal checkout than when you are doing a
> "initial checkout".

When you are starting from commit H and checking out a different
commit M, and when a path in the index does not match what is
recorded in commit H, usually Git tries to keep the state of the
path you have in your index as a "local change", as long as the data
recorded for the path is the same between H and M.  A path in the
index that matches what is recorded in commit H and with different
data recorded for it in commit M gets M's version in the index and
the working file is updated to match (but it requires that either
the working tree file is missing, or the working tree version
matches what is in the original index, to avoid data loss).

But imagine you have just cloned and are trying to finish that
process.  What Git has done so far would include creating an empty
repository, populating the object database and pointing branches at
various commits.  HEAD now points at the branch (usually 'master'),
the index file does not exist (you haven't checked out anything),
and we want to populate the index and the working tree files to
match what is recorded in HEAD.  We do so by starting from commit
HEAD and checking out commit HEAD.

This situation presents conflicting goals to the above "keep the
local change" rule.  To the rule, this situation looks as if you
removed each and every path from the index (as the index hasn't been
populated yet---in fact, the index file does not even exist yet in
this state), but the data recorded for each path are the same
between commit H and commit M (as H==M==HEAD in this case), so "keep
the local change" rule would leave the index and the working tree
empty X-<.

That is rescued by the "initial checkout behaves differently and
forces the index and the working tree match what is recorded in
commit M" exception.  It probably should be obvious to the readers
by now that the absense of .git/index is used as the clue for this
exception to kick in from the above use case.

And that is exactly the condition that is checked by
read-cache.c::is_index_unborn().

Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert

On Tue, Nov 06, 2018 at 05:11:18PM -0500, Jeff King wrote:

> On Tue, Nov 06, 2018 at 06:48:22PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> > The implementation looks fine to me, but as noted in
> > https://public-inbox.org/git/8736se6qyc@evledraar.gmail.com/ I
> > wonder what the plausible end-game is. That we'll turn this on by
> > default in a few years, and then you can only worry about
> > git-interpret-trailers for repos created as of 2020 or something?
> > 
> > Otherwise it seems we'll need to *also* parse out the existing messages
> > we've added.
> 
> Could we help the reading scripts by normalizing old and new output via
> interpret-trailers, %(trailers), etc?
> 
> I think "(cherry picked from ...)" is already considered a trailer by
> the trailer code. If the caller instructs us to, we could probably
> rewrite it to:
> 
>   Cherry-picked-from: ...
> 
> in the output. Then the end-game is that scripts should just use
> interpret-trailers, etc, and old and new commits will Just Work.

I.e., something like this:

diff --git a/trailer.c b/trailer.c
index 0796f326b3..491c7c240e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -46,9 +46,11 @@ static int configured;
 
 #define TRAILER_ARG_STRING "$ARG"
 
+#define CHERRY_PICK_PREFIX "(cherry picked from commit "
+
 static const char *git_generated_prefixes[] = {
"Signed-off-by: ",
-   "(cherry picked from commit ",
+   CHERRY_PICK_PREFIX,
NULL
 };
 
@@ -1141,6 +1143,8 @@ static void format_trailer_info(struct strbuf *out,
for (i = 0; i < info->trailer_nr; i++) {
char *trailer = info->trailers[i];
ssize_t separator_pos = find_separator(trailer, separators);
+   const char *hex;
+   struct object_id oid;
 
if (separator_pos >= 1) {
struct strbuf tok = STRBUF_INIT;
@@ -1154,6 +1158,12 @@ static void format_trailer_info(struct strbuf *out,
strbuf_release();
strbuf_release();
 
+   } else if (/* should check opts->normalize_cherry_pick */1 &&
+  skip_prefix(trailer, CHERRY_PICK_PREFIX, ) &&
+  !get_oid_hex(hex, )) {
+   strbuf_addf(out, "Cherry-picked-from: %s\n",
+   oid_to_hex());
+
} else if (!opts->only_trailers) {
strbuf_addstr(out, trailer);
}

That would need to add the normalize_cherry_pick flag to the options
struct, and then plumb it through. But it's enough to play around with,
and:

  git log --format='%h%n%(trailers:only)'

works.

-Peff

Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert

On Tue, Nov 06, 2018 at 06:48:22PM +0100, Ævar Arnfjörð Bjarmason wrote:

> The implementation looks fine to me, but as noted in
> https://public-inbox.org/git/8736se6qyc@evledraar.gmail.com/ I
> wonder what the plausible end-game is. That we'll turn this on by
> default in a few years, and then you can only worry about
> git-interpret-trailers for repos created as of 2020 or something?
> 
> Otherwise it seems we'll need to *also* parse out the existing messages
> we've added.

Could we help the reading scripts by normalizing old and new output via
interpret-trailers, %(trailers), etc?

I think "(cherry picked from ...)" is already considered a trailer by
the trailer code. If the caller instructs us to, we could probably
rewrite it to:

  Cherry-picked-from: ...

in the output. Then the end-game is that scripts should just use
interpret-trailers, etc, and old and new commits will Just Work.

-Peff

Re: [PATCH] gitk: don't highlight submodule diff lines outside submodule diffs

2018-11-06 Thread Роман Донченко


06.11.2018 23:06, Stefan Beller пишет:

On Tue, Nov 6, 2018 at 12:03 PM Роман Донченко  wrote:


A line that starts with "  <" or "  >" is not necessarily a submodule
diff line. It might just be a context line in a normal diff, representing
a line starting with " <" or " >" respectively.

Use the currdiffsubmod variable to track whether we are currently
inside a submodule diff and only highlight these lines if we are.


This explanation makes sense, some prior art is at
https://public-inbox.org/git/20181021163401.4458-1-du...@example.com/
which was not taken AFAICT.


Didn't see that patch. That said, I think it's incorrect, since it never 
resets currdiffsubmod back to the empty string, so if a normal diff 
follows a submodule diff, the same issue will occur.


(The `set $currdiffsubmod ""` lines that are already there are 
effectively useless because they set the variable whose name is the 
contents of currdiffsubmod, rather than currdiffsubmod itself. I assume

it was a typo.)

-Roman



Thanks,
Stefan



Signed-off-by: Роман Донченко 
---
  gitk | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gitk b/gitk
index a14d7a1..6bb6dc6 100755
--- a/gitk
+++ b/gitk
@@ -8109,6 +8109,8 @@ proc parseblobdiffline {ids line} {
 }
 # start of a new file
 set diffinhdr 1
+   set currdiffsubmod ""
+
 $ctext insert end "\n"
 set curdiffstart [$ctext index "end - 1c"]
 lappend ctext_file_names ""
@@ -8191,12 +8193,10 @@ proc parseblobdiffline {ids line} {
 } else {
 $ctext insert end "$line\n" filesep
 }
-} elseif {![string compare -length 3 "  >" $line]} {
-   set $currdiffsubmod ""
+} elseif {$currdiffsubmod ne "" && ![string compare -length 3 "  >" 
$line]} {
 set line [encoding convertfrom $diffencoding $line]
 $ctext insert end "$line\n" dresult
-} elseif {![string compare -length 3 "  <" $line]} {
-   set $currdiffsubmod ""
+} elseif {$currdiffsubmod ne "" && ![string compare -length 3 "  <" 
$line]} {
 set line [encoding convertfrom $diffencoding $line]
 $ctext insert end "$line\n" d0
  } elseif {$diffinhdr} {
--
2.19.1.windows.1

Re: [PATCH 1/1] Windows: force-recompile git.res for differing architectures

2018-11-06 Thread Johannes Sixt


Am 06.11.18 um 15:55 schrieb Johannes Schindelin via GitGitGadget:

From: Johannes Schindelin 

When git.rc is compiled into git.res, the result is actually dependent
on the architecture. That is, you cannot simply link a 32-bit git.res
into a 64-bit git.exe.

Therefore, to allow 32-bit and 64-bit builds in the same directory, we
let git.res depend on GIT-PREFIX so that it gets recompiled when
compiling for a different architecture (this works because the exec path
changes based on the architecture: /mingw32/libexec/git-core for 32-bit
and /mingw64/libexec/git-core for 64-bit).


On Linux, when I recompile for a different architecture, CFLAGS would 
change, so I would have thought that GIT-CFLAGS were the natural choice 
for a dependency. Don't they change in this case on Windows, too?




Signed-off-by: Johannes Schindelin 
---
  Makefile | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index bbfbb4292d..8375736c32 100644
--- a/Makefile
+++ b/Makefile
@@ -2110,7 +2110,7 @@ $(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
$(QUIET_GEN)$(cmd_munge_script) && \
mv $@+ $@
  
-git.res: git.rc GIT-VERSION-FILE

+git.res: git.rc GIT-VERSION-FILE GIT-PREFIX
$(QUIET_RC)$(RC) \
  $(join -DMAJOR= -DMINOR= -DMICRO= -DPATCHLEVEL=, $(wordlist 1, 4, \
$(shell echo $(GIT_VERSION) 0 0 0 0 | tr '.a-zA-Z-' ' '))) \

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

2018-11-06 Thread Johannes Sixt


Am 06.11.18 um 15:53 schrieb Johannes Schindelin via GitGitGadget:

From: Johannes Schindelin 

On Windows, an absolute POSIX path needs to be turned into a Windows
one.


If I were picky, I would say that in a pure Windows application there 
cannot be POSIX paths to begin with.


Even if a path looks like a POSIX paths, i.e. it starts with a directory 
separator, but not with drive-letter-colon, it still has a particular 
meaning, namely (as far as I know) that the path is anchored at the root 
of the drive of the current working directory.


If a user specifies such a path on Windows, and it must undergo 
expand_user_path(), then that is a user error, or the user knows what 
they are doing.


I think it is wrong to interpret such a path as relative to some random 
other directory, like this patch seems to do.




Signed-off-by: Johannes Schindelin 
---
  path.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/path.c b/path.c
index 34f0f98349..a72abf0e1f 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
  #include "path.h"
  #include "packfile.h"
  #include "object-store.h"
+#include "exec-cmd.h"
  
  static int get_st_mode_bits(const char *path, int *mode)

  {
@@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
  
  	if (path == NULL)

goto return_null;
+#ifdef __MINGW32__
+   if (path[0] == '/')
+   return system_path(path + 1);
+#endif
if (path[0] == '~') {
const char *first_slash = strchrnul(path, '/');
const char *username = path + 1;

Re: What exactly is a "initial checkout"

On Tue, Nov 06, 2018 at 01:38:45PM +0100, Christian Halstrick wrote:

> I am trying to teach JGit [1] to behave like native git regarding some
> corner cases during "git checkout". I am reading the "git read-tree"
> documentation and I am not sure about the case [2]. Git should behave
> differently during a normal checkout than when you are doing a
> "initial checkout". I can imagine that the first checkout you do after
> you have cloned a repo is a initial checkout but: What exactly defines
> a "initial checkout"? It can't be an empty or non-existing index
> because native git behaves like in a non-initial-checkout even if the
> index is empty (see example below).
> 
> Here are some commands explaining my case. Git is facing an empty
> index, HEAD and MERGE (the commit you checkout) have the some content
> for path 'p'  and still git is neither updating index nor workingtree
> file during checkout.

Without looking at the code, I'd assume that an empty HEAD is different
than "there is no HEAD at all". I.e., what we call an "unborn branch"
elsewhere.

So perhaps try:

  git init
  git fetch ../some/other/repo HEAD:tmp
  git checkout tmp

where you'd truly have no HEAD.

Though peeking at the code, it looks like we set the unpack_trees
initial_checkout flag based on is_cache_unborn(), which looks for a
totally missing index file (not just an empty one). That would trigger
in the above case, too, though, because of course we have no index there
either.

-Peff

Re: [PATCH v2 1/1] pack-objects: ignore ambiguous object warnings

On Tue, Nov 06, 2018 at 12:34:47PM -0800, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee 
> 
> A git push process runs several processes during its run, but one
> includes git send-pack which calls git pack-objects and passes
> the known have/wants into stdin using object ids. However, the
> default setting for core.warnAmbiguousRefs requires git pack-objects
> to check for ref names matching the ref_rev_parse_rules array in
> refs.c. This means that every object is triggering at least six
> "file exists?" queries.  When there are a lot of refs, this can
> add up significantly! I observed a simple push spending three
> seconds checking these paths.

Thanks, this fills out the details from the cover letter a bit better.

> The fix here is similar to 4c30d50 "rev-list: disable object/refname
> ambiguity check with --stdin". Save the value of the global
> warn_on_object_refname_ambiguity variable (which is usually set to
> the boolean config variable core.warnAmbiguousRefs) and change the
> state to false. Do this only during the get_object_list() method
> which reads the objects from stdin.

I think this parenthetical isn't quite right. We never change the value
of warn_on_object_refname_ambiguity based on user config. It's just that
if warn_ambiguous_refs is off, we do not even bother looking at the
object_refname variant.

So we'd never expect to see anything except "1" in our save_warning
variable. Doing a save/restore is just about code hygiene and
maintainability.

Other than that minor nit, this whole thing looks good to me.

-Peff

[PATCH v2 1/1] pack-objects: ignore ambiguous object warnings

From: Derrick Stolee 

A git push process runs several processes during its run, but one
includes git send-pack which calls git pack-objects and passes
the known have/wants into stdin using object ids. However, the
default setting for core.warnAmbiguousRefs requires git pack-objects
to check for ref names matching the ref_rev_parse_rules array in
refs.c. This means that every object is triggering at least six
"file exists?" queries.  When there are a lot of refs, this can
add up significantly! I observed a simple push spending three
seconds checking these paths.

The fix here is similar to 4c30d50 "rev-list: disable object/refname
ambiguity check with --stdin". Save the value of the global
warn_on_object_refname_ambiguity variable (which is usually set to
the boolean config variable core.warnAmbiguousRefs) and change the
state to false. Do this only during the get_object_list() method
which reads the objects from stdin.

Helped-by: Jeff King 
Signed-off-by: Derrick Stolee 
---
 builtin/pack-objects.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d1144a8f7e..f703e6df9b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2988,6 +2988,7 @@ static void get_object_list(int ac, const char **av)
struct rev_info revs;
char line[1000];
int flags = 0;
+   int save_warning;
 
init_revisions(, NULL);
save_commit_buffer = 0;
@@ -2996,6 +2997,9 @@ static void get_object_list(int ac, const char **av)
/* make sure shallows are read */
is_repository_shallow(the_repository);
 
+   save_warning = warn_on_object_refname_ambiguity;
+   warn_on_object_refname_ambiguity = 0;
+
while (fgets(line, sizeof(line), stdin) != NULL) {
int len = strlen(line);
if (len && line[len - 1] == '\n')
@@ -3022,6 +3026,8 @@ static void get_object_list(int ac, const char **av)
die(_("bad revision '%s'"), line);
}
 
+   warn_on_object_refname_ambiguity = save_warning;
+
if (use_bitmap_index && !get_object_list_from_bitmap())
return;
 
-- 
gitgitgadget

[PATCH v2 0/1] send-pack: set core.warnAmbiguousRefs=false

I've been looking into the performance of git push for very large repos. Our
users are reporting that 60-80% of git push time is spent during the
"Enumerating objects" phase of git pack-objects.

A git push process runs several processes during its run, but one includes
git send-pack which calls git pack-objects and passes the known have/wants
into stdin using object ids. However, the default setting for
core.warnAmbiguousRefs requires git pack-objects to check for ref names
matching the ref_rev_parse_rules array in refs.c. This means that every
object is triggering at least six "file exists?" queries.

When there are a lot of refs, this can add up significantly! My PerfView
trace for a simple push measured 3 seconds spent checking these paths.

The fix is to set the global warn_on_object_refname_ambiguity to 0 for the
section that is performing these object reads.

In addition to this patch submission, we are looking into merging it into
our fork sooner [1].

[1] https://github.com/Microsoft/git/pull/67

Changes in V2: Instead of using the "-c" flag from send-pack, just set the
global. I left the name of the cover letter the same to not confuse anyone
viewing the message without threading.

Derrick Stolee (1):
pack-objects: ignore ambiguous object warnings

builtin/pack-objects.c | 6 ++
1 file changed, 6 insertions(+)

base-commit: cae598d9980661a978e2df4fb338518f7bf09572
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-68%2Fderrickstolee%2Fsend-pack-config-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-68/derrickstolee/send-pack-config-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/68

Range-diff vs v1:

1: 1ef2c51550 < -: -- send-pack: set core.warnAmbiguousRefs=false
-: -- > 1: 002868ee6b pack-objects: ignore ambiguous object warnings

--
gitgitgadget

Re: [PATCH 0/1] send-pack: set core.warnAmbiguousRefs=false

2018-11-06 Thread Derrick Stolee


On 11/6/2018 2:51 PM, Jeff King wrote:

On Tue, Nov 06, 2018 at 02:44:42PM -0500, Jeff King wrote:


The fix for this is simple: set core.warnAmbiguousRefs to false for this
specific call of git pack-objects coming from git send-pack. We don't want
to default it to false for all calls to git pack-objects, as it is valid to
pass ref names instead of object ids. This helps regain these seconds during
a push.

I don't think you actually care about the ambiguity check between refs
here; you just care about avoiding the ref check when we've seen (and
are mostly expecting) a 40-hex sha1. We have a more specific flag for
that: warn_on_object_refname_ambiguity.

And I think it would be OK to enable that all the time for pack-objects,
which is plumbing that does typically expect object names. See prior art
in 25fba78d36 (cat-file: disable object/refname ambiguity check for
batch mode, 2013-07-12) and 4c30d50402 (rev-list: disable object/refname
ambiguity check with --stdin, 2014-03-12).

I'd probably do it here:

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index e50c6cd1ff..d370638a5d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3104,6 +3104,7 @@ static void get_object_list(int ac, const char **av)


Scoping the change into get_object_list does make sense. I was doing it 
a level higher, which is not worth it. I'll reproduce your change here.



struct rev_info revs;
char line[1000];
int flags = 0;
+   int save_warning;
  
  	repo_init_revisions(the_repository, , NULL);

save_commit_buffer = 0;
@@ -3112,6 +3113,9 @@ static void get_object_list(int ac, const char **av)
/* make sure shallows are read */
is_repository_shallow(the_repository);
  
+	save_warning = warn_on_object_refname_ambiguity;

+   warn_on_object_refname_ambiguity = 0;
+
while (fgets(line, sizeof(line), stdin) != NULL) {
int len = strlen(line);
if (len && line[len - 1] == '\n')
@@ -3138,6 +3142,8 @@ static void get_object_list(int ac, const char **av)
die(_("bad revision '%s'"), line);
}
  
+	warn_on_object_refname_ambiguity = save_warning;

+
if (use_bitmap_index && !get_object_list_from_bitmap())
return;
  


But I'll leave it to you to wrap that up in a patch, since you probably
should re-check your timings (which it would be interesting to include
in the commit message, if you have reproducible timings).


The timings change a lot depending on the disk cache and the remote 
refs, which is unfortunate, but I have measured a three-second improvement.


Thanks,
-Stolee

Re: git-rebase is ignoring working-tree-encoding

2018-11-06 Thread Torsten Bögershausen

On Mon, Nov 05, 2018 at 07:10:14PM +0100, Torsten Bögershausen wrote:
> On Mon, Nov 05, 2018 at 05:24:39AM +0100, Adrián Gimeno Balaguer wrote:
> 
> []
> 
> > https://github.com/git/git/pull/550
>  
> []
>  
> > This is covered in the mentioned PR above. Thanks for feedback.
> 
> Thanks for the code,
> I will have a look (the next days)
> 
> > 
> > -- 
> > Adrián

Hej Adrián,

I still didn't manage to fully understand your problem.
I tried to convert your test into my understanding,
It can be fetched here (or copied from this message, see below)

https://github.com/tboegi/git/tree/tb.181106_UTF16LE_commit

The commit of an empty file seems to work for me, in the initial
report a "rebase" was mentioned, which is not in the TC ?

Is the following what you intended to test ?

#!/bin/sh
test_description='UTF-16 LE/BE file encoding using working-tree-encoding'


. ./test-lib.sh

# We specify the UTF-16LE BOM manually, to not depend on programs such as iconv.
utf16leBOM=$(printf '\377\376')

test_expect_success 'Stage empty UTF-16LE file as binary' '
>empty_0.txt &&
echo "empty_0.txt binary" >>.gitattributes &&
git add empty_0.txt
'


test_expect_success 'Stage empty file with enc=UTF.16BL' '
>utf16le_0.txt &&
echo "utf16le_0.txt text working-tree-encoding=UTF-16BE" 
>>.gitattributes &&
git add utf16le_0.txt
'


test_expect_success 'Create and stage UTF-16LE file with only BOM' '
printf "$utf16leBOM" >utf16le_1.txt &&
echo "utf16le_1.txt text working-tree-encoding=UTF-16" >>.gitattributes 
&&
git add utf16le_1.txt
'

test_expect_success 'Dont stage UTF-16LE file with only BOM with enc=UTF.16BE' '
printf "$utf16leBOM" >utf16le_2.txt &&
echo "utf16le_2.txt text working-tree-encoding=UTF-16BE" 
>>.gitattributes &&
test_must_fail git add utf16le_2.txt
'

test_expect_success 'commit all files' '
test_tick &&
git commit -m "Commit all 3 files"
'

test_expect_success 'All commited files have the same sha' '
git ls-files -s --eol >tmp1 &&
sed -e "s!  i/none.*!!" actual &&
>expect &&
test_cmp expect actual
'

test_done

Re: [PATCH] gitk: don't highlight submodule diff lines outside submodule diffs

2018-11-06 Thread Stefan Beller

On Tue, Nov 6, 2018 at 12:03 PM Роман Донченко  wrote:
>
> A line that starts with "  <" or "  >" is not necessarily a submodule
> diff line. It might just be a context line in a normal diff, representing
> a line starting with " <" or " >" respectively.
>
> Use the currdiffsubmod variable to track whether we are currently
> inside a submodule diff and only highlight these lines if we are.

This explanation makes sense, some prior art is at
https://public-inbox.org/git/20181021163401.4458-1-du...@example.com/
which was not taken AFAICT.

Thanks,
Stefan

>
> Signed-off-by: Роман Донченко 
> ---
>  gitk | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/gitk b/gitk
> index a14d7a1..6bb6dc6 100755
> --- a/gitk
> +++ b/gitk
> @@ -8109,6 +8109,8 @@ proc parseblobdiffline {ids line} {
> }
> # start of a new file
> set diffinhdr 1
> +   set currdiffsubmod ""
> +
> $ctext insert end "\n"
> set curdiffstart [$ctext index "end - 1c"]
> lappend ctext_file_names ""
> @@ -8191,12 +8193,10 @@ proc parseblobdiffline {ids line} {
> } else {
> $ctext insert end "$line\n" filesep
> }
> -} elseif {![string compare -length 3 "  >" $line]} {
> -   set $currdiffsubmod ""
> +} elseif {$currdiffsubmod ne "" && ![string compare -length 3 "  >" 
> $line]} {
> set line [encoding convertfrom $diffencoding $line]
> $ctext insert end "$line\n" dresult
> -} elseif {![string compare -length 3 "  <" $line]} {
> -   set $currdiffsubmod ""
> +} elseif {$currdiffsubmod ne "" && ![string compare -length 3 "  <" 
> $line]} {
> set line [encoding convertfrom $diffencoding $line]
> $ctext insert end "$line\n" d0
>  } elseif {$diffinhdr} {
> --
> 2.19.1.windows.1
>

[PATCH] gitk: don't highlight submodule diff lines outside submodule diffs

2018-11-06 Thread Роман Донченко

A line that starts with "  <" or "  >" is not necessarily a submodule
diff line. It might just be a context line in a normal diff, representing
a line starting with " <" or " >" respectively.

Use the currdiffsubmod variable to track whether we are currently
inside a submodule diff and only highlight these lines if we are.

Signed-off-by: Роман Донченко 
---
 gitk | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gitk b/gitk
index a14d7a1..6bb6dc6 100755
--- a/gitk
+++ b/gitk
@@ -8109,6 +8109,8 @@ proc parseblobdiffline {ids line} {
}
# start of a new file
set diffinhdr 1
+   set currdiffsubmod ""
+
$ctext insert end "\n"
set curdiffstart [$ctext index "end - 1c"]
lappend ctext_file_names ""
@@ -8191,12 +8193,10 @@ proc parseblobdiffline {ids line} {
} else {
$ctext insert end "$line\n" filesep
}
-} elseif {![string compare -length 3 "  >" $line]} {
-   set $currdiffsubmod ""
+} elseif {$currdiffsubmod ne "" && ![string compare -length 3 "  >" 
$line]} {
set line [encoding convertfrom $diffencoding $line]
$ctext insert end "$line\n" dresult
-} elseif {![string compare -length 3 "  <" $line]} {
-   set $currdiffsubmod ""
+} elseif {$currdiffsubmod ne "" && ![string compare -length 3 "  <" 
$line]} {
set line [encoding convertfrom $diffencoding $line]
$ctext insert end "$line\n" d0
 } elseif {$diffinhdr} {
-- 
2.19.1.windows.1

Re: [PATCH 0/1] send-pack: set core.warnAmbiguousRefs=false

2018-11-06 Thread Derrick Stolee


On 11/6/2018 2:44 PM, Jeff King wrote:

On Tue, Nov 06, 2018 at 11:13:47AM -0800, Derrick Stolee via GitGitGadget wrote:


I've been looking into the performance of git push for very large repos. Our
users are reporting that 60-80% of git push time is spent during the
"Enumerating objects" phase of git pack-objects.

A git push process runs several processes during its run, but one includes
git send-pack which calls git pack-objects and passes the known have/wants
into stdin using object ids. However, the default setting for
core.warnAmbiguousRefs requires git pack-objects to check for ref names
matching the ref_rev_parse_rules array in refs.c. This means that every
object is triggering at least six "file exists?" queries.

When there are a lot of refs, this can add up significantly! My PerfView
trace for a simple push measured 3 seconds spent checking these paths.

Some of this might be useful in the commit message. :)


The fix for this is simple: set core.warnAmbiguousRefs to false for this
specific call of git pack-objects coming from git send-pack. We don't want
to default it to false for all calls to git pack-objects, as it is valid to
pass ref names instead of object ids. This helps regain these seconds during
a push.

I don't think you actually care about the ambiguity check between refs
here; you just care about avoiding the ref check when we've seen (and
are mostly expecting) a 40-hex sha1. We have a more specific flag for
that: warn_on_object_refname_ambiguity.

And I think it would be OK to enable that all the time for pack-objects,
which is plumbing that does typically expect object names. See prior art
in 25fba78d36 (cat-file: disable object/refname ambiguity check for
batch mode, 2013-07-12) and 4c30d50402 (rev-list: disable object/refname
ambiguity check with --stdin, 2014-03-12).
Thanks for these pointers. Helps to know there is precedent for shutting 
down the

behavior without relying on "-c" flags.


Whenever I see a change like this to the pack-objects invocation for
send-pack, it makes me wonder if upload-pack would want the same thing.

It's a moot point if we just set the flag directly in inside
pack-objects, though.

I'll send a v2 that does just that.

Thanks,
-Stolee

Re: [PATCH 0/1] send-pack: set core.warnAmbiguousRefs=false

On Tue, Nov 06, 2018 at 02:44:42PM -0500, Jeff King wrote:

> > The fix for this is simple: set core.warnAmbiguousRefs to false for this
> > specific call of git pack-objects coming from git send-pack. We don't want
> > to default it to false for all calls to git pack-objects, as it is valid to
> > pass ref names instead of object ids. This helps regain these seconds during
> > a push.
> 
> I don't think you actually care about the ambiguity check between refs
> here; you just care about avoiding the ref check when we've seen (and
> are mostly expecting) a 40-hex sha1. We have a more specific flag for
> that: warn_on_object_refname_ambiguity.
> 
> And I think it would be OK to enable that all the time for pack-objects,
> which is plumbing that does typically expect object names. See prior art
> in 25fba78d36 (cat-file: disable object/refname ambiguity check for
> batch mode, 2013-07-12) and 4c30d50402 (rev-list: disable object/refname
> ambiguity check with --stdin, 2014-03-12).

I'd probably do it here:

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index e50c6cd1ff..d370638a5d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3104,6 +3104,7 @@ static void get_object_list(int ac, const char **av)
struct rev_info revs;
char line[1000];
int flags = 0;
+   int save_warning;
 
repo_init_revisions(the_repository, , NULL);
save_commit_buffer = 0;
@@ -3112,6 +3113,9 @@ static void get_object_list(int ac, const char **av)
/* make sure shallows are read */
is_repository_shallow(the_repository);
 
+   save_warning = warn_on_object_refname_ambiguity;
+   warn_on_object_refname_ambiguity = 0;
+
while (fgets(line, sizeof(line), stdin) != NULL) {
int len = strlen(line);
if (len && line[len - 1] == '\n')
@@ -3138,6 +3142,8 @@ static void get_object_list(int ac, const char **av)
die(_("bad revision '%s'"), line);
}
 
+   warn_on_object_refname_ambiguity = save_warning;
+
if (use_bitmap_index && !get_object_list_from_bitmap())
return;
 

But I'll leave it to you to wrap that up in a patch, since you probably
should re-check your timings (which it would be interesting to include
in the commit message, if you have reproducible timings).

-Peff

Re: [PATCH 0/1] send-pack: set core.warnAmbiguousRefs=false

On Tue, Nov 06, 2018 at 11:13:47AM -0800, Derrick Stolee via GitGitGadget wrote:

> I've been looking into the performance of git push for very large repos. Our
> users are reporting that 60-80% of git push time is spent during the
> "Enumerating objects" phase of git pack-objects.
> 
> A git push process runs several processes during its run, but one includes 
> git send-pack which calls git pack-objects and passes the known have/wants
> into stdin using object ids. However, the default setting for 
> core.warnAmbiguousRefs requires git pack-objects to check for ref names
> matching the ref_rev_parse_rules array in refs.c. This means that every
> object is triggering at least six "file exists?" queries.
> 
> When there are a lot of refs, this can add up significantly! My PerfView
> trace for a simple push measured 3 seconds spent checking these paths.

Some of this might be useful in the commit message. :)

> The fix for this is simple: set core.warnAmbiguousRefs to false for this
> specific call of git pack-objects coming from git send-pack. We don't want
> to default it to false for all calls to git pack-objects, as it is valid to
> pass ref names instead of object ids. This helps regain these seconds during
> a push.

I don't think you actually care about the ambiguity check between refs
here; you just care about avoiding the ref check when we've seen (and
are mostly expecting) a 40-hex sha1. We have a more specific flag for
that: warn_on_object_refname_ambiguity.

And I think it would be OK to enable that all the time for pack-objects,
which is plumbing that does typically expect object names. See prior art
in 25fba78d36 (cat-file: disable object/refname ambiguity check for
batch mode, 2013-07-12) and 4c30d50402 (rev-list: disable object/refname
ambiguity check with --stdin, 2014-03-12).

> Derrick Stolee (1):
>   send-pack: set core.warnAmbiguousRefs=false
> 
>  send-pack.c | 2 ++
>  1 file changed, 2 insertions(+)

Whenever I see a change like this to the pack-objects invocation for
send-pack, it makes me wonder if upload-pack would want the same thing.

It's a moot point if we just set the flag directly in inside
pack-objects, though.

-Peff

Re: Regression in rebase-in-C with rebase.autoStash=true

On Tue, Nov 06, 2018 at 02:59:43PM +0100, Ævar Arnfjörð Bjarmason wrote:

> 
> On Tue, Oct 23 2018, Johannes Schindelin via GitGitGadget wrote:
> 
> > Johannes Schindelin (2):
> >   rebase --autostash: demonstrate a problem with dirty submodules
> >   rebase --autostash: fix issue with dirty submodules
> >
> >  builtin/rebase.c|  2 +-
> >  t/t3420-rebase-autostash.sh | 10 ++
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> There's another bug with rebase.autoStash in master (and next/pu) but
> not v2.19.0. I tried to bisect bit it just comes down to 5541bd5b8f
> ("rebase: default to using the builtin rebase", 2018-08-08).

That's just flipping the config default. If you add "-c
rebase.usebuiltin=true" to your invocation here:

> git -c rebase.autoStash=true -c pull.rebase=true pull &&

you can bisect further. However, the results weren't super useful, as I
had to skip a bunch of commits (ones that did die("TODO") or just
complained that the working tree wasn't clean; if you treat the latter
as "bad", then it just bisects to e0333e5c63 (builtin rebase: require a
clean worktree, 2018-09-04).

-Peff

[PATCH 1/1] send-pack: set core.warnAmbiguousRefs=false

From: Derrick Stolee 

During a 'git push' command, we run 'git send-pack' inside of our
transport helper. This creates a 'git pack-objects' process and
passes a list of object ids. If this list is large, then the
pack-objects process can spend a lot of time checking the possible
refs these strings could represent.

Remove this extra check by setting core.warnAmbiguousRefs to false
as we call 'git pack-objects'.

Signed-off-by: Derrick Stolee 
---
 send-pack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/send-pack.c b/send-pack.c
index e920ca57df..5055150fe1 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -64,6 +64,8 @@ static int pack_objects(int fd, struct ref *refs, struct 
oid_array *extra, struc
int i;
int rc;
 
+   argv_array_push(, "-c");
+   argv_array_push(, "core.warnAmbiguousRefs=false");
argv_array_push(, "pack-objects");
argv_array_push(, "--all-progress-implied");
argv_array_push(, "--revs");
-- 
gitgitgadget

[PATCH 0/1] send-pack: set core.warnAmbiguousRefs=false

I've been looking into the performance of git push for very large repos. Our
users are reporting that 60-80% of git push time is spent during the
"Enumerating objects" phase of git pack-objects.

When there are a lot of refs, this can add up significantly! My PerfView
trace for a simple push measured 3 seconds spent checking these paths.

The fix for this is simple: set core.warnAmbiguousRefs to false for this
specific call of git pack-objects coming from git send-pack. We don't want
to default it to false for all calls to git pack-objects, as it is valid to
pass ref names instead of object ids. This helps regain these seconds during
a push.

In addition to this patch submission, we are looking into merging it into
our fork sooner [1].

[1] https://github.com/Microsoft/git/pull/67

Derrick Stolee (1):
send-pack: set core.warnAmbiguousRefs=false

send-pack.c | 2 ++
1 file changed, 2 insertions(+)

base-commit: cae598d9980661a978e2df4fb338518f7bf09572
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-68%2Fderrickstolee%2Fsend-pack-config-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-68/derrickstolee/send-pack-config-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/68
--
gitgitgadget

Re: [PATCH v2 12/16] parse-options: replace opterror() with optname()

On Tue, Nov 06, 2018 at 02:02:42PM +, Ramsay Jones wrote:

> Also, this patch does not replace opterror() calls outside of
> the 'parse-options.c' file with optname(). This tickles my
> static-check.pl script, since optname() is an external function
> which is only called from 'parse-options.c'.
> 
> So, at present, optname() could be marked as a local 'static'
> symbol. However, I could also imagine it being used by new callers
> outside of 'parse-options.c' in the future. (maybe) Your call. ;-)

One potential caller is the BUG() cases I added in:

  https://public-inbox.org/git/20181105064542.gm25...@sigill.intra.peff.net/

I actually thought about factoring out an optname() there, but saw that
it involved memory ownership issues and punted (since those messages are
just BUG()s anyway, and unlikely to be triggered). But if we have it, we
could use it.

-Peff

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

On Tue, Nov 6, 2018 at 7:15 PM Ramsay Jones  wrote:
> >> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int 
> >> real_home)
> >>
> >>  if (path == NULL)
> >>  goto return_null;
> >> +#ifdef __MINGW32__
> >> +if (path[0] == '/')
> >> +return system_path(path + 1);
> >> +#endif
> >
> > Hmm, this doesn't quite fit with the intended use of this
> > function! ;-) (even on windows!)
> >
> > I haven't looked very deeply, but doesn't this affect all
> > absolute paths in the config read by git_config_pathname(),
> > along with all 'included config' files?
> >
> > I am pretty sure that I would not want the absolute paths
> > in my config file(s) magically 'moved' depending on whether
> > git has been compiled with 'runtime prefix' support or not!
>
> So, I hit 'send' before finishing my thought ...
>
> I can't think of a non-backwards compatible way of doing
> what you want. If backward compatibility wasn't an issue,
> then we could (maybe) have used some kind of pathname prefix
> like 'system:/path/relative/to/git/executable', or somesuch.

A pseudo variable might work, like $ROOT/path/relative/to/somewhere
-- 
Duy

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

On Tue, Nov 6, 2018 at 3:55 PM Johannes Schindelin via GitGitGadget
 wrote:
>
> From: Johannes Schindelin 
>
> On Windows, an absolute POSIX path needs to be turned into a Windows
> one.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  path.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/path.c b/path.c
> index 34f0f98349..a72abf0e1f 100644
> --- a/path.c
> +++ b/path.c
> @@ -11,6 +11,7 @@
>  #include "path.h"
>  #include "packfile.h"
>  #include "object-store.h"
> +#include "exec-cmd.h"
>
>  static int get_st_mode_bits(const char *path, int *mode)
>  {
> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>
> if (path == NULL)
> goto return_null;
> +#ifdef __MINGW32__
> +   if (path[0] == '/')
> +   return system_path(path + 1);
> +#endif

Should this behavior be documented somewhere, maybe in config.txt?

> if (path[0] == '~') {
> const char *first_slash = strchrnul(path, '/');
> const char *username = path + 1;
> --
> gitgitgadget
-- 
Duy

Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert

On Tue, Nov 06 2018, Nguyễn Thái Ngọc Duy wrote:

> OK here is a less constroversal attempt to add new trailers. Instead
> of changing the default behavior (which could be done incrementally
> later), this patch simply adds a new option --append-trailer to revert
> and cherry-pick.
>
> Both will show either
>
> Reverts: [~]
>
> or
>
> Cherry-picked-from: [~]
>
> --append-trailer could be added to more commands (e.g. merge) that
> generate commit messages if they have something for machine
> consumption.
>
> After this, perhaps we could have a config key to turn this on by
> default (for revert; for cherry-pick it will turn off "-x" too). Then
> after a couple releases, the we got good reception, we'll make it
> default?
>
> No tests, no proper commit message since I think we're still going to
> discuss a bit more before settling down.
>
> PS. maybe --append-trailer is too generic? We have --signoff which is
> also a trailer. But that one carries more weights than just "machine
> generated tags".
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 

The implementation looks fine to me, but as noted in
https://public-inbox.org/git/8736se6qyc@evledraar.gmail.com/ I
wonder what the plausible end-game is. That we'll turn this on by
default in a few years, and then you can only worry about
git-interpret-trailers for repos created as of 2020 or something?

Otherwise it seems we'll need to *also* parse out the existing messages
we've added.

Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

On Tue, Nov 06 2018, Duy Nguyen wrote:

> On Tue, Nov 6, 2018 at 9:57 AM Ævar Arnfjörð Bjarmason  
> wrote:
>> Leaving aside the question of whether the pain of switching is worth it,
>> I think it's a worthwihle to consider if we could stop hardcoding one
>> specific human language in commit messages, and instead leave something
>> machine-readable behind.
>>
>> We do that with reverts, and also with merge commits, which could be
>> given a similar treatment where we change e.g. "Merge branches
>> 'jc/convert', 'jc/bigfile' and 'jc/replacing' into jc/streaming" (to use
>> git.git's 02071b27f1 as an example) to:
>>
>> Merge-branch-1: jc/convert
>> Merge-branch-2: jc/bigfile
>> Merge-branch-3: jc/replacing
>> Merge-branch-into: jc/streaming
>>
>> Then, when rendering the commit in the UI we could parse that out, and
>> put a "Merge branches[...]" message at the top, except this time in the
>> user's own language.
>
> My first reaction of this was "but branch name is a local thing and
> not significant anyway!". But then if people use one branch as a
> bundle of other branches like 'pu', then the ability to recreate
> branches (with the right name of course) from those merges may be
> useful. So... yeah, maybe.

Yeah, in theory, in practice no. But all I'm observing is that we're
*already* encoding this information, so to me at least the logical end
game to changes like what you're proposing is something like the above,
otherwise why bother?

But having thought about it a bit more I wonder if
git-interpret-trailers (or some command like it) shouldn't just as a
special-case learn to do more parsing of commit messages we've
historically generated.

E.g. know how to parse out a merge message we've produced and spew out
something like the above Merge-* output. Same for existing "Revert"
output, if anyone cares about parsing this they'll need to do that
anyway.

Re: [PATCH] format-patch: respect --stat when explicitly specified

On Tue, Nov 6, 2018 at 6:25 PM Leif Lindholm  wrote:
> > > Sure. Only today was the first time I had a look at the git sources,
> > > so some guidance would be most appreciated.
> >
> > No problem (and if you don't have time to do it, just say the word and
> > I will continue; this is my bug after all)
>
> Wll, if you're offering, I would certainly appreciate not having
> to dig deeper into this. I've got a patch review backlog the length of
> my arm in another project...

Your loss. Your name is not going to be in git.git then (j/k). Thanks
for the report. I'll double, triple check, add tests and resubmit
soon.
-- 
Duy

Re: [PATCH] format-patch: respect --stat when explicitly specified

2018-11-06 Thread Leif Lindholm

On Tue, Nov 06, 2018 at 06:13:00PM +0100, Duy Nguyen wrote:
> On Tue, Nov 6, 2018 at 5:31 PM Leif Lindholm  wrote:
> > > > diff --git a/builtin/log.c b/builtin/log.c
> > > > index 061d4fd86..07e6ae2c1 100644
> > > > --- a/builtin/log.c
> > > > +++ b/builtin/log.c
> > > > @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,
> > > >
> > > > memcpy(, >diffopt, sizeof(opts));
> > > > opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> > > > -   opts.stat_width = MAIL_DEFAULT_WRAP;
> > > > +   if (rev->diffopt.stat_width == -1)
> > >
> > > I don't think we get -1 here when stat_width is not defined. The
> > > "undefined" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP
> > > in here unless you specify --stat.
> >
> > From what I could tell, if nothing is specified on command line,
> > rev->diffopt.stat_width is set to -1 at this point (I assumed by
> > rev->cmd_log_init_defaults(), but didn't go digging).
> 
> I thought the same but could find where cmd_log_.. is called by
> format-patch. I was not even sure if I read the code correctly so I
> ran the command through gdb. It was definitely not called.

Huh...

> > The patched version certainly gives the <= 2.16.* behaviour with
> > --stat and still restricts stat lines to 72 characters without.
> >
> > > So I think you can just drop the below assignment. But if you want to
> > > be on the safe side, check for zero stat_width instead of -1 and set
> > > MAIL_DEFAULT_WRAP.
> > >
> > > > +   opts.stat_width = MAIL_DEFAULT_WRAP;
> > >
> > > How about a test to make sure this will not be broken in future?
> >
> > Sure. Only today was the first time I had a look at the git sources,
> > so some guidance would be most appreciated.
> 
> No problem (and if you don't have time to do it, just say the word and
> I will continue; this is my bug after all)

Wll, if you're offering, I would certainly appreciate not having
to dig deeper into this. I've got a patch review backlog the length of
my arm in another project...

> > Should I add a function to t/t4014-format-patch.sh and just put
> > something longer than a/file for the expect template?
> 
> First of all the README file in that directory could give pretty good
> basic instructions.
> 
> Back to this test, I think you could start by copying to a new test
> (the whole "test_expect_success" block, optionally including the
> "expect" file creation too), add --stat there and see what it looks
> like. For stat testing, t4052 could also be a good example. Or perhaps
> the test should be added in t4052 because it already supports long
> file name ($name is 120 char long).

(Thanks!)

/
Leif

[PATCH/RFC] Support --append-trailer in cherry-pick and revert

2018-11-06 Thread Nguyễn Thái Ngọc Duy

OK here is a less constroversal attempt to add new trailers. Instead
of changing the default behavior (which could be done incrementally
later), this patch simply adds a new option --append-trailer to revert
and cherry-pick.

Both will show either

Reverts: [~]

or

Cherry-picked-from: [~]

--append-trailer could be added to more commands (e.g. merge) that
generate commit messages if they have something for machine
consumption.

After this, perhaps we could have a config key to turn this on by
default (for revert; for cherry-pick it will turn off "-x" too). Then
after a couple releases, the we got good reception, we'll make it
default?

No tests, no proper commit message since I think we're still going to
discuss a bit more before settling down.

PS. maybe --append-trailer is too generic? We have --signoff which is
also a trailer. But that one carries more weights than just "machine
generated tags".

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-cherry-pick.txt |  6 +
 Documentation/git-revert.txt  |  6 +
 builtin/revert.c  |  7 ++
 sequencer.c   | 39 +++
 sequencer.h   |  1 +
 5 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt 
b/Documentation/git-cherry-pick.txt
index d35d771fc8..b5dff29ead 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -102,6 +102,12 @@ effect to your index in a row.
Add Signed-off-by line at the end of the commit message.
See the signoff option in linkgit:git-commit[1] for more information.
 
+--append-trailer::
+   When recording a commit, append a "Cherry-picked-from:" line
+   with object name of the cherry-picked commit. If a merge is
+   cherry-picked with `-m`, the extended SHA-1 syntax is used
+   to indicate the side of the merge to be cherry-picked.
+
 -S[]::
 --gpg-sign[=]::
GPG-sign commits. The `keyid` argument is optional and
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 837707a8fd..e08010b200 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -91,6 +91,12 @@ effect to your index in a row.
Add Signed-off-by line at the end of the commit message.
See the signoff option in linkgit:git-commit[1] for more information.
 
+--append-trailer::
+   When recording a commit, append a "Cherry-picked-from:" line
+   with object name of the cherry-picked commit. If a merge is
+   cherry-picked with `-m`, the extended SHA-1 syntax is used
+   to indicate the side of the merge to be cherry-picked.
+
 --strategy=::
Use the given merge strategy.  Should only be used once.
See the MERGE STRATEGIES section in linkgit:git-merge[1]
diff --git a/builtin/revert.c b/builtin/revert.c
index c93393c89b..3bd8f57b90 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -119,6 +119,7 @@ static int run_sequencer(int argc, const char **argv, 
struct replay_opts *opts)
if (opts->action == REPLAY_PICK) {
struct option cp_extra[] = {
OPT_BOOL('x', NULL, >record_origin, N_("append 
commit name")),
+   OPT_BOOL(0, "append-trailer", >append_trailer, 
N_("record cherry picked commit as trailer")),
OPT_BOOL(0, "ff", >allow_ff, N_("allow 
fast-forward")),
OPT_BOOL(0, "allow-empty", >allow_empty, 
N_("preserve initially empty commits")),
OPT_BOOL(0, "allow-empty-message", 
>allow_empty_message, N_("allow commits with empty messages")),
@@ -126,6 +127,12 @@ static int run_sequencer(int argc, const char **argv, 
struct replay_opts *opts)
OPT_END(),
};
options = parse_options_concat(options, cp_extra);
+   } else if (opts->action == REPLAY_REVERT) {
+   struct option cp_extra[] = {
+   OPT_BOOL(0, "append-trailer", >append_trailer, 
N_("record reverted commit as trailer")),
+   OPT_END(),
+   };
+   options = parse_options_concat(options, cp_extra);
}
 
argc = parse_options(argc, argv, NULL, options, usage_str,
diff --git a/sequencer.c b/sequencer.c
index 9e1ab3a2a7..e8fa307109 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -911,7 +911,7 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
if ((flags & EDIT_MSG))
argv_array_push(, "-e");
else if (!(flags & CLEANUP_MSG) &&
-!opts->signoff && !opts->record_origin &&
+!opts->signoff && !opts->record_origin && 
!opts->append_trailer &&
 git_config_get_value("commit.cleanup", ))
argv_array_push(, "--cleanup=verbatim");
 
@@ -1669,7 +1669,7 @@ static int do_pick_commit(enum todo_command command, 
struct commit

Re: [PATCH] format-patch: respect --stat when explicitly specified

On Tue, Nov 6, 2018 at 5:31 PM Leif Lindholm  wrote:
> > > diff --git a/builtin/log.c b/builtin/log.c
> > > index 061d4fd86..07e6ae2c1 100644
> > > --- a/builtin/log.c
> > > +++ b/builtin/log.c
> > > @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,
> > >
> > > memcpy(, >diffopt, sizeof(opts));
> > > opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> > > -   opts.stat_width = MAIL_DEFAULT_WRAP;
> > > +   if (rev->diffopt.stat_width == -1)
> >
> > I don't think we get -1 here when stat_width is not defined. The
> > "undefined" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP
> > in here unless you specify --stat.
>
> From what I could tell, if nothing is specified on command line,
> rev->diffopt.stat_width is set to -1 at this point (I assumed by
> rev->cmd_log_init_defaults(), but didn't go digging).

I thought the same but could find where cmd_log_.. is called by
format-patch. I was not even sure if I read the code correctly so I
ran the command through gdb. It was definitely not called.

> The patched version certainly gives the <= 2.16.* behaviour with
> --stat and still restricts stat lines to 72 characters without.
>
> > So I think you can just drop the below assignment. But if you want to
> > be on the safe side, check for zero stat_width instead of -1 and set
> > MAIL_DEFAULT_WRAP.
> >
> > > +   opts.stat_width = MAIL_DEFAULT_WRAP;
> >
> > How about a test to make sure this will not be broken in future?
>
> Sure. Only today was the first time I had a look at the git sources,
> so some guidance would be most appreciated.

No problem (and if you don't have time to do it, just say the word and
I will continue; this is my bug after all)

> Should I add a function to t/t4014-format-patch.sh and just put
> something longer than a/file for the expect template?

First of all the README file in that directory could give pretty good
basic instructions.

Back to this test, I think you could start by copying to a new test
(the whole "test_expect_success" block, optionally including the
"expect" file creation too), add --stat there and see what it looks
like. For stat testing, t4052 could also be a good example. Or perhaps
the test should be added in t4052 because it already supports long
file name ($name is 120 char long).
-- 
Duy

[ANNOUNCE] StGit 0.19

2018-11-06 Thread Peter Grayson

I am pleased to announce the release of Stacked Git 0.19.

The big feature for this release is Python 3 support, but 0.19 also
contains some important bug fixes and more robust test infrastructure.

The full release notes follow.

Cheers,
Pete

%<

   Stacked Git 0.19 released
   -

StGit is a Python application providing functionality similar to Quilt
(i.e. pushing/popping patches to/from a stack) on top of Git. These
operations are performed using Git commands, and the patches are stored
as Git commit objects, allowing easy merging of the StGit patches into
other repositories using standard Git functionality.

  Download: https://github.com/ctmarinas/stgit/archive/v0.19.tar.gz
  Main repository:  https://github.com/ctmarinas/stgit
  Project homepage: http://www.procode.org/stgit/
  Issue tracker:https://github.com/ctmarinas/stgit/issues

The main changes since release 0.18:

- Python 3 support. StGit supports Python 2.6, 2.7, 3.3, 3.4, 3.5, 3.6,
  and 3.7. PyPy interpreters are also supported.

- Submodules are now ignored when checking if working tree is clean.
  Submodules are also not included by default when refreshing a patch.

- Config booleans are now parsed similarly to git-config.

- contrib/stgit.el is now licenced with GPLv2.

- Repair handling of emails with utf-8 bodies containing latin-1
  characters.  Also correctly decode email headers containing quoted
  encoded words.

- StGit's version is now correct/available in the release archive.

- Add continuous integration (travis-ci) and code coverage (coveralls)
  support.

- Many new test cases were added.

Re: [PATCH] format-patch: respect --stat when explicitly specified

2018-11-06 Thread Leif Lindholm

On Tue, Nov 06, 2018 at 04:56:11PM +0100, Duy Nguyen wrote:
> On Tue, Nov 6, 2018 at 11:48 AM Leif Lindholm  
> wrote:
> >
> > Commit 43662b23abbd
> > ("format-patch: keep cover-letter diffstat wrapped in 72 columns") made
> > format-patch keep the diffstat to within 72 characters. However, it does
> > this even when --stat is explicitly set on the command line.
> >
> > Make it possible to explicitly override the new mechanism, using --stat,
> > matching the functionality before this change. This also matches the
> > output in the case of non-cover-letter files.
> >
> > Cc: Nguyễn Thái Ngọc Duy 
> > Cc: Junio C Hamano 
> > Reported-by: Laszlo Ersek 
> > Signed-off-by: Leif Lindholm 
> > ---
> >
> > Note:
> > In TianoCore we have LotsOfGloriousFilesNamedInReallyLongCamelCase, so
> > our official submission guidelines specify the use of --stat.
> >
> >  builtin/log.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/log.c b/builtin/log.c
> > index 061d4fd86..07e6ae2c1 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,
> >
> > memcpy(, >diffopt, sizeof(opts));
> > opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> > -   opts.stat_width = MAIL_DEFAULT_WRAP;
> > +   if (rev->diffopt.stat_width == -1)
> 
> I don't think we get -1 here when stat_width is not defined. The
> "undefined" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP
> in here unless you specify --stat.

>From what I could tell, if nothing is specified on command line,
rev->diffopt.stat_width is set to -1 at this point (I assumed by
rev->cmd_log_init_defaults(), but didn't go digging).

The patched version certainly gives the <= 2.16.* behaviour with
--stat and still restricts stat lines to 72 characters without.

> So I think you can just drop the below assignment. But if you want to
> be on the safe side, check for zero stat_width instead of -1 and set
> MAIL_DEFAULT_WRAP.
> 
> > +   opts.stat_width = MAIL_DEFAULT_WRAP;
> 
> How about a test to make sure this will not be broken in future?

Sure. Only today was the first time I had a look at the git sources,
so some guidance would be most appreciated.

Should I add a function to t/t4014-format-patch.sh and just put
something longer than a/file for the expect template?

/
Leif

> >
> > diff_setup_done();
> >
> > --
> > 2.11.0
> -- 
> Duy

[PATCH v2] range-diff: add a --no-patch option to show a summary

Add a --no-patch option which shows which changes got removed, added
or moved etc., without showing the diff associated with them.

This allows for using range-diff as a poor man's "shortlog" for
force-pushed branches to see what changed without getting into the
details of what specifically. E.g. diffing the latest forced-push to
"pu" gives us:

$ ./git-range-diff --no-patch b58974365b...711aaa392f | head -n 10
 -:  -- >  1:  b613de67c4 diff: differentiate error handling in 
parse_color_moved_ws
28:  c731affab0 !  2:  23c4bbe28e build: link with curl-defined linker flags
 -:  -- >  3:  14f74d5907 git-worktree.txt: correct linkgit command 
name
 -:  -- >  4:  29d51e214c sequencer.c: remove a stray semicolon
 -:  -- >  5:  b7845cebc0 tree-walk.c: fix overoptimistic inclusion 
in :(exclude) matching
 -:  -- >  6:  1a550529b1 t/t7510-signed-commit.sh: Add %GP to 
custom format checks
 -:  -- >  7:  1e690847d1 t/t7510-signed-commit.sh: add signing 
subkey to Eris Discordia key
 9:  d13ecb7d81 !  8:  d8ad847421 Add a base implementation of SHA-256 
support
10:  3f0382eef8 =  9:  cdae1d391c sha256: add an SHA-256 implementation 
using libgcrypt
11:  2422fd4227 = 10:  7d81aa0857 hash: add an SHA-256 implementation using 
OpenSSL

That would print a total of 44 lines of output, but the full
range-diff output with --patch is 460 lines.

I thought of implementing --stat too. It would be neat if passing
DIFF_FORMAT_DIFFSTAT just worked, but using that shows the underlying
implementation details of how range-diff works, instead of a useful
diffstat. So I'll leave that to a future change. Such a feature should
be something like a textual summary of the --patch output itself,
e.g.:

N hunks, X insertions(+), Y deletions(-)

This change doesn't update git-format-patch with a --no-patch
option. That can be added later similar to how format-patch first
learned --range-diff, and then --creation-factor in
8631bf1cdd ("format-patch: add --creation-factor tweak for
--range-diff", 2018-07-22). See [1] and related E-Mails for a
discussion about that.

1. https://public-inbox.org/git/87d0ri7gbs@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
Range-diff:
1:  6e735e991c ! 1:  fe4e251f26 range-diff: add a --no-patch option to show a 
summary
@@ -38,8 +38,10 @@
 option. That can be added later similar to how format-patch first
 learned --range-diff, and then --creation-factor in
 8631bf1cdd ("format-patch: add --creation-factor tweak for
---range-diff", 2018-07-22). I don't see why anyone would want this for
-format-patch, it pretty much defeats the point of range-diff.
+--range-diff", 2018-07-22). See [1] and related E-Mails for a
+discussion about that.
+
+1. https://public-inbox.org/git/87d0ri7gbs@evledraar.gmail.com/
 
 Signed-off-by: Ævar Arnfjörð Bjarmason 
 
@@ -50,9 +52,10 @@
See the ``Algorithm`` section below for an explanation why this is
needed.
  
++-s::
 +--no-patch::
-+  Don't show the range-diff itself, only which patches are the
-+  same or were added or removed etc.
++  Suppress diff output. Only shows how the range has changed at
++  a commit-level.
 +
   ::
Compare the commits specified by the two ranges, where
@@ -78,14 +81,14 @@
int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
struct diff_options diffopt = { NULL };
int simple_color = -1;
-+  int patch = 1;
++  int no_patch = 0;
struct option options[] = {
OPT_INTEGER(0, "creation-factor", _factor,
N_("Percentage by which creation is weighted")),
OPT_BOOL(0, "no-dual-color", _color,
N_("use simple diff colors")),
-+  OPT_BOOL('p', "patch", ,
-+   N_("show patch output")),
++  OPT_BOOL_F('s', "no-patch", _patch,
++   N_("show patch output"), PARSE_OPT_NONEG),
OPT_END()
};
int i, j, res = 0;
@@ -94,7 +97,7 @@
  
res = show_range_diff(range1.buf, range2.buf, creation_factor,
 -simple_color < 1, );
-+simple_color < 1, patch, );
++simple_color < 1, !no_patch, );
  
strbuf_release();
strbuf_release();
@@ -155,7 +158,14 @@
test_cmp expected actual
  '
  
-+test_expect_success 'changed commit --no-patch' '
++test_expect_success 'changed commit -p & --patch' '
++  git range-diff --no-color -p topic...changed >actual &&
++  test_cmp expected actual &&
++  git range-diff --no-color --patch topic...changed >actual &&
++  test_cmp expected actual
++'
++
++test_expect_success 'changed commit -s &

Re: [PATCH] format-patch: respect --stat when explicitly specified

On Tue, Nov 6, 2018 at 5:18 PM Laszlo Ersek  wrote:
> >> +   opts.stat_width = MAIL_DEFAULT_WRAP;
> >
> > How about a test to make sure this will not be broken in future?
>
> Oh, looks like I won't have to test this patch at all! ;)
>
> (Just kidding, I'll test the next iteration.)

Just to be clear I didn't mean you didn't test it. I meant adding a
new test case to our test suite.
-- 
Duy

Re: [PATCH] format-patch: respect --stat when explicitly specified

2018-11-06 Thread Laszlo Ersek

On 11/06/18 16:56, Duy Nguyen wrote:
> On Tue, Nov 6, 2018 at 11:48 AM Leif Lindholm  
> wrote:
>>
>> Commit 43662b23abbd
>> ("format-patch: keep cover-letter diffstat wrapped in 72 columns") made
>> format-patch keep the diffstat to within 72 characters. However, it does
>> this even when --stat is explicitly set on the command line.
>>
>> Make it possible to explicitly override the new mechanism, using --stat,
>> matching the functionality before this change. This also matches the
>> output in the case of non-cover-letter files.
>>
>> Cc: Nguyễn Thái Ngọc Duy 
>> Cc: Junio C Hamano 
>> Reported-by: Laszlo Ersek 
>> Signed-off-by: Leif Lindholm 
>> ---
>>
>> Note:
>> In TianoCore we have LotsOfGloriousFilesNamedInReallyLongCamelCase, so
>> our official submission guidelines specify the use of --stat.
>>
>>  builtin/log.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 061d4fd86..07e6ae2c1 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,
>>
>> memcpy(, >diffopt, sizeof(opts));
>> opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
>> -   opts.stat_width = MAIL_DEFAULT_WRAP;
>> +   if (rev->diffopt.stat_width == -1)
> 
> I don't think we get -1 here when stat_width is not defined. The
> "undefined" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP
> in here unless you specify --stat.
> 
> So I think you can just drop the below assignment. But if you want to
> be on the safe side, check for zero stat_width instead of -1 and set
> MAIL_DEFAULT_WRAP.

Looks like I'll have to test v2 then...

> 
>> +   opts.stat_width = MAIL_DEFAULT_WRAP;
> 
> How about a test to make sure this will not be broken in future?

Oh, looks like I won't have to test this patch at all! ;)

(Just kidding, I'll test the next iteration.)

Thanks,
Laszlo

> 
>>
>> diff_setup_done();
>>
>> --
>> 2.11.0

Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

On Tue, Nov 6, 2018 at 9:57 AM Ævar Arnfjörð Bjarmason  wrote:
> Leaving aside the question of whether the pain of switching is worth it,
> I think it's a worthwihle to consider if we could stop hardcoding one
> specific human language in commit messages, and instead leave something
> machine-readable behind.
>
> We do that with reverts, and also with merge commits, which could be
> given a similar treatment where we change e.g. "Merge branches
> 'jc/convert', 'jc/bigfile' and 'jc/replacing' into jc/streaming" (to use
> git.git's 02071b27f1 as an example) to:
>
> Merge-branch-1: jc/convert
> Merge-branch-2: jc/bigfile
> Merge-branch-3: jc/replacing
> Merge-branch-into: jc/streaming
>
> Then, when rendering the commit in the UI we could parse that out, and
> put a "Merge branches[...]" message at the top, except this time in the
> user's own language.

My first reaction of this was "but branch name is a local thing and
not significant anyway!". But then if people use one branch as a
bundle of other branches like 'pu', then the ability to recreate
branches (with the right name of course) from those merges may be
useful. So... yeah, maybe.
-- 
Duy

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

2018-11-06 Thread Ramsay Jones




On 06/11/2018 15:54, Ramsay Jones wrote:
> 
> 
> On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin 
>>
>> On Windows, an absolute POSIX path needs to be turned into a Windows
>> one.
>>
>> Signed-off-by: Johannes Schindelin 
>> ---
>>  path.c | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/path.c b/path.c
>> index 34f0f98349..a72abf0e1f 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -11,6 +11,7 @@
>>  #include "path.h"
>>  #include "packfile.h"
>>  #include "object-store.h"
>> +#include "exec-cmd.h"
>>  
>>  static int get_st_mode_bits(const char *path, int *mode)
>>  {
>> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>>  
>>  if (path == NULL)
>>  goto return_null;
>> +#ifdef __MINGW32__
>> +if (path[0] == '/')
>> +return system_path(path + 1);
>> +#endif
> 
> Hmm, this doesn't quite fit with the intended use of this
> function! ;-) (even on windows!)
> 
> I haven't looked very deeply, but doesn't this affect all
> absolute paths in the config read by git_config_pathname(),
> along with all 'included config' files?
> 
> I am pretty sure that I would not want the absolute paths
> in my config file(s) magically 'moved' depending on whether
> git has been compiled with 'runtime prefix' support or not!

So, I hit 'send' before finishing my thought ...

I can't think of a non-backwards compatible way of doing
what you want. If backward compatibility wasn't an issue,
then we could (maybe) have used some kind of pathname prefix
like 'system:/path/relative/to/git/executable', or somesuch.

ATB,
Ramsay Jones

Re: Understanding pack format

On Tue, Nov 6, 2018 at 3:23 AM Farhan Khan  wrote:
> To follow-up from the other day, I have been reading the code that
> retrieves the pack entry for the past 3 days now without much success.
> But there are quite a few abstractions and I get lost half-way down
> the line.

Jeff already gave you some pointers. This is just a side note.

I think it's easier to run the code under a debugger and see what it
does than just reading it. You can create a repo with just one blob to
have better control over it (small packs also make it possible to
examine with a hex editor in parallel), e.g.

git init foo
cd foo
echo hello >file
git add file
git repack -ad
gdb --args git show :./file

then put a breakpoint in some interesting functions (perhaps one of
those Jeff pointed out)
-- 
Duy

Re: [PATCH] range-diff: add a --no-patch option to show a summary



On Tue, Nov 06 2018, Johannes Schindelin wrote:

> Hi,
>
> On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> On Mon, Nov 05 2018, Eric Sunshine wrote:
>>
>> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason  
>> > wrote:
>> >> Add a --no-patch option which shows which changes got removed, added
>> >> or moved etc., without showing the diff associated with them.
>> >
>> > This option existed in the very first version[1] of range-diff (then
>> > called branch-diff) implemented by Dscho, although it was called
>> > --no-patches (with an "es"), which it inherited from tbdiff. I think
>> > someone (possibly me) pointed out that --no-patch (sans "es") would be
>> > more consistent with existing Git options. I don't recall why Dscho
>> > removed the option during the re-rolls, but the explanation may be in
>> > that thread.
>>
>> Thanks for digging. Big thread, not going to re-read it now. I'd just
>> like to have this.
>
> In my hands, the well-documented `-s` option works (see e.g.
> https://git-scm.com/docs/git-diff#git-diff--s), although I have to admit
> that the `git-range-diff` manual does not talk about the diff-options.
>
> And for the record, for me, `git range-diff A...B --no-patch` *already*
> works.

Neither of those works for me without my patch. E.g.

./git-range-diff -s 711aaa392f...a5ba8f2101
./git-range-diff --no-patch 711aaa392f...a5ba8f2101

This is on current next, 2.19.1.1182.g4ecb1133ce. What version are you
on?

>>
>> > I was also wondering if --summarize or --summary-only might be a
>> > better name, describing the behavior at a higher level, but since
>> > there is precedent for --no-patch (or --no-patches in tbdiff), perhaps
>> > the name is fine as is.
>>
>> I think we should aim to keep a 1=1 mapping between range-diff and
>> log/show options when possible, even though the output might have a
>> slightly different flavor as my 4th paragraph discussing a potential
>> --stat talks about.
>>
>> E.g. I can imagine that range-diff --no-patch --stat --summary would not
>> show the patch, but a stat as described there, plus e.g. a "create
>> mode..." if applicable.
>>
>> This change implements only a tiny fraction of that, but it would be
>> very neat if we supported more stuff, and showed it in range-diff-y way,
>> e.g. some compact format showing:
>>
>> 1 file changed, 3->2 insertions(+), 10->9 deletions(-)
>> create mode 100(6 -> 7)44 new-executable
>>
>> > The patch itself looks okay.
>> >
>> > [1]: 
>> > https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schinde...@gmx.de/
>>

Re: [PATCH] format-patch: respect --stat when explicitly specified

On Tue, Nov 6, 2018 at 11:48 AM Leif Lindholm  wrote:
>
> Commit 43662b23abbd
> ("format-patch: keep cover-letter diffstat wrapped in 72 columns") made
> format-patch keep the diffstat to within 72 characters. However, it does
> this even when --stat is explicitly set on the command line.
>
> Make it possible to explicitly override the new mechanism, using --stat,
> matching the functionality before this change. This also matches the
> output in the case of non-cover-letter files.
>
> Cc: Nguyễn Thái Ngọc Duy 
> Cc: Junio C Hamano 
> Reported-by: Laszlo Ersek 
> Signed-off-by: Leif Lindholm 
> ---
>
> Note:
> In TianoCore we have LotsOfGloriousFilesNamedInReallyLongCamelCase, so
> our official submission guidelines specify the use of --stat.
>
>  builtin/log.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 061d4fd86..07e6ae2c1 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,
>
> memcpy(, >diffopt, sizeof(opts));
> opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> -   opts.stat_width = MAIL_DEFAULT_WRAP;
> +   if (rev->diffopt.stat_width == -1)

I don't think we get -1 here when stat_width is not defined. The
"undefined" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP
in here unless you specify --stat.

So I think you can just drop the below assignment. But if you want to
be on the safe side, check for zero stat_width instead of -1 and set
MAIL_DEFAULT_WRAP.

> +   opts.stat_width = MAIL_DEFAULT_WRAP;

How about a test to make sure this will not be broken in future?

>
> diff_setup_done();
>
> --
> 2.11.0
-- 
Duy

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

2018-11-06 Thread Ramsay Jones

On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin 
> 
> On Windows, an absolute POSIX path needs to be turned into a Windows
> one.
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  path.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/path.c b/path.c
> index 34f0f98349..a72abf0e1f 100644
> --- a/path.c
> +++ b/path.c
> @@ -11,6 +11,7 @@
>  #include "path.h"
>  #include "packfile.h"
>  #include "object-store.h"
> +#include "exec-cmd.h"
>  
>  static int get_st_mode_bits(const char *path, int *mode)
>  {
> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>  
>   if (path == NULL)
>   goto return_null;
> +#ifdef __MINGW32__
> + if (path[0] == '/')
> + return system_path(path + 1);
> +#endif

Hmm, this doesn't quite fit with the intended use of this
function! ;-) (even on windows!)

I haven't looked very deeply, but doesn't this affect all
absolute paths in the config read by git_config_pathname(),
along with all 'included config' files?

I am pretty sure that I would not want the absolute paths
in my config file(s) magically 'moved' depending on whether
git has been compiled with 'runtime prefix' support or not!

ATB,
Ramsay Jones

>   if (path[0] == '~') {
>   const char *first_slash = strchrnul(path, '/');
>   const char *username = path + 1;
>

Re: Checkout deleted semi-untracked file



On Tue, Nov 06 2018, Steffen Jost wrote:

> Hello!
>
> A brief discussion on the git user mailing list on Google Groups recommended 
> me to file the following as a bug report.
>
> The problem led to an actual file loss, but I suspect that this might be 
> intended:
>
> 1) .gitignore is added to the repository (which then causes problems)
> 2) A file is added, repeatedly edited and comitted to a remote repository.
> 3) Later on, the file is added to .gitignore and "git rm --cached file" is 
> executed (since the file now contains information private to each developer).
> 4) Several commits happen.
> 5) I checkout an old branch which has not yet seen the change in .gitignore 
> in the master branch. The file is reverted to the state of the branch.
> 6) I checkout master, and the file with all later changes is irrevocably lost.
>
> I usually advise my students to check-in their .gitignore file into the 
> repository. Apparently this is a bad advice, since it now led to a somewhat 
> painful file loss for me.
>
> So what is the actual advice on this? Google turned up mixed advice there, 
> and the git user mailing list on Google Groups recommended me submitting this 
> as a bug here. However, I think this works as intended. However, I don't know 
> either how to avert this problem, apart from not checking in the .gitignore 
> file (or checking it in under a different name and copying it manually).

This recent thread should be a good starting point:
https://public-inbox.org/git/4c6a1c5b.4030...@workspacewhiz.com/

My reply here I think has an overview of some of the caveats:
https://public-inbox.org/git/871s8qdzph@evledraar.gmail.com/

tl;dr: Git assumes that a pattern in .gitignore means you don't care
about the contents, since it's meant for *.o and the like. This leads to
data loss in some situations (such as yours).

Various suggestions in that thread of ways forward, but none have
been implemented>

[PATCH 0/1] Windows: force-recompile git.res for differing architectures

2018-11-06 Thread Johannes Schindelin via GitGitGadget

This is a patch designed to help maintaining Git for Windows better: when
the same source code is "cross-compiled" for i686 as well as x86_64, we want
to rebuild the whole thing, including the resource file git.res.

Note: regular C files are re-compiled appropriately, as the default prefix
in Git for Windows is /mingw32 or /mingw64 depending on the architecture,
and this difference is manifested in the CFLAGS (which, upon change, trigger
a complete rebuild).

As non-Windows platforms do not even compile these .res files, this patch
should have exactly no effect on non-Windows builds.

Johannes Schindelin (1):
  Windows: force-recompile git.res for differing architectures

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: cd69ec8cde54af1817630331fc441f493866f0d4
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-67%2Fdscho%2Fmingw-git.res-bitness-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-67/dscho/mingw-git.res-bitness-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/67
-- 
gitgitgadget

[PATCH 1/1] Windows: force-recompile git.res for differing architectures

2018-11-06 Thread Johannes Schindelin via GitGitGadget

From: Johannes Schindelin 

When git.rc is compiled into git.res, the result is actually dependent
on the architecture. That is, you cannot simply link a 32-bit git.res
into a 64-bit git.exe.

Therefore, to allow 32-bit and 64-bit builds in the same directory, we
let git.res depend on GIT-PREFIX so that it gets recompiled when
compiling for a different architecture (this works because the exec path
changes based on the architecture: /mingw32/libexec/git-core for 32-bit
and /mingw64/libexec/git-core for 64-bit).

Signed-off-by: Johannes Schindelin 
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index bbfbb4292d..8375736c32 100644
--- a/Makefile
+++ b/Makefile
@@ -2110,7 +2110,7 @@ $(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
$(QUIET_GEN)$(cmd_munge_script) && \
mv $@+ $@
 
-git.res: git.rc GIT-VERSION-FILE
+git.res: git.rc GIT-VERSION-FILE GIT-PREFIX
$(QUIET_RC)$(RC) \
  $(join -DMAJOR= -DMINOR= -DMICRO= -DPATCHLEVEL=, $(wordlist 1, 4, \
$(shell echo $(GIT_VERSION) 0 0 0 0 | tr '.a-zA-Z-' ' '))) \
-- 
gitgitgadget

[PATCH 1/1] mingw: handle absolute paths in expand_user_path()

2018-11-06 Thread Johannes Schindelin via GitGitGadget

From: Johannes Schindelin 

On Windows, an absolute POSIX path needs to be turned into a Windows
one.

Signed-off-by: Johannes Schindelin 
---
 path.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/path.c b/path.c
index 34f0f98349..a72abf0e1f 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
 #include "path.h"
 #include "packfile.h"
 #include "object-store.h"
+#include "exec-cmd.h"
 
 static int get_st_mode_bits(const char *path, int *mode)
 {
@@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
 
if (path == NULL)
goto return_null;
+#ifdef __MINGW32__
+   if (path[0] == '/')
+   return system_path(path + 1);
+#endif
if (path[0] == '~') {
const char *first_slash = strchrnul(path, '/');
const char *username = path + 1;
-- 
gitgitgadget

[PATCH 0/1] mingw: handle absolute paths in expand_user_path()

2018-11-06 Thread Johannes Schindelin via GitGitGadget

While this patch has been "in production" in Git for Windows for a good
while, this patch series is half meant as a request for comments.

The reason is this: something like this (make paths specified e.g. via 
http.sslCAInfo relative to the runtime prefix) is potentially useful also in
the non-Windows context, as long as Git was built with the runtime prefix
feature.

The main problem with non-Windows platforms is that paths starting with a
single slash are unambiguously absolute, whereas we can say with certainty
that they are not absolute Windows paths.

So maybe someone on this list has a clever idea how we could specify paths
(unambiguously, even on non-Windows platforms) that Git should interpret as
relative to the runtime prefix?

Johannes Schindelin (1):
  mingw: handle absolute paths in expand_user_path()

 path.c | 5 +
 1 file changed, 5 insertions(+)


base-commit: cd69ec8cde54af1817630331fc441f493866f0d4
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-66%2Fdscho%2Fmingw-expand-absolute-user-path-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-66/dscho/mingw-expand-absolute-user-path-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/66
-- 
gitgitgadget

Why Me In This Terrible Condition.

2018-11-06 Thread Mrs Franisca carlsen

Greetings My Dear,

I sent this mail praying it will found you in a good condition of
health, since I myself are in a very critical health condition in
which I  sleep every night without knowing if I may be alive to see
the next day. I am Mrs. Francisca  Carlsen from Denmark wife of late
Mr John Carlsen, a widow suffering from long time illness. I have some
funds I inherited from my late husband, the sum of (eleven million
dollars) my Doctor told me recently that I have serious sickness which
is cancer problem. What disturbs me most is my stroke sickness. Having
known my condition, I decided to donate this fund to a good person
that will utilize it the way i am going to instruct herein. I need a
very honest and God fearing person who can claim this money and use it
for Charity works, for orphanages, widows and also build schools for
less privileges that will be named after my late husband if possible
and to promote the word of God and the effort that the house of God is
maintained.

I do not want a situation where this money will be used in an ungodly
manner. That's why I'm taking this decision. I'm not afraid of death,
so I know where I'm going. I accept this decision because I do not
have any child who will inherit this money after I die. Please I want
your sincerely and urgent answer to know if you will be able to
execute this project, and I will give you more information on how the
fund will be transferred to your bank account. I am waiting for your
reply.

May God Bless you,
Mrs. Francisca Carlsen

Re: [PATCH v2 12/16] parse-options: replace opterror() with optname()

2018-11-06 Thread Ramsay Jones




On 06/11/2018 02:33, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy   writes:
> 
>> There are a few issues with opterror()
>>
>> - it tries to assemble an English sentence from pieces. This is not
>>   great for translators because we give them pieces instead of a full
>>   sentence.
>>
>> - It's a wrapper around error() and needs some hack to let the
>>   compiler know it always returns -1.
>>
>> - Since it takes a string instead of printf format, one call site has
>>   to assemble the string manually before passing to it.
>>
>> Kill it and produce the option name with optname(). The user will use
>> error() directly. This solves the second and third problems.
> 
> The proposed log message is not very friendly to reviewers, as there
> is no hint what optname() does nor where it came from; it turns out
> that this patch introduces it.
> 
> Introduce optname() that does the early half of original
> opterror() to come up with the name of the option reported back
> to the user, and use it to kill opterror().  The callers of
> opterror() now directly call error() using the string returned
> by opterror() instead.
> 
> or something like that perhaps.
> 
> Theoretically not very friendly to topics in flight, but I do not
> expect there would be any right now that wants to add new callers of
> opterror().
> 
> I do agree with the reasoning behind this change.  Thanks for
> working on it.
> 

Also, this patch does not replace opterror() calls outside of
the 'parse-options.c' file with optname(). This tickles my
static-check.pl script, since optname() is an external function
which is only called from 'parse-options.c'.

So, at present, optname() could be marked as a local 'static'
symbol. However, I could also imagine it being used by new callers
outside of 'parse-options.c' in the future. (maybe) Your call. ;-)

ATB,
Ramsay Jones

Re: [PATCH v2 1/1] diff-highlight: Use correct /dev/null for UNIX and Windows

2018-11-06 Thread Johannes Schindelin

List,

I have no idea why this mail made it to GitGitGadget's email account but
not to the Git mailing list... Sorry about that.

Ciao,
Johannes

On Wed, 31 Oct 2018, Chris. Webster via GitGitGadget wrote:

> From: "Chris. Webster" 
> 
> Use File::Spec->devnull() for output redirection to avoid messages
> when Windows version of Perl is first in path.  The message 'The
> system cannot find the path specified.' is displayed each time git is
> run to get colors.
> 
> Signed-off-by: Chris. Webster 
> ---
>  contrib/diff-highlight/DiffHighlight.pm | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/diff-highlight/DiffHighlight.pm 
> b/contrib/diff-highlight/DiffHighlight.pm
> index 536754583b..7440aa1c46 100644
> --- a/contrib/diff-highlight/DiffHighlight.pm
> +++ b/contrib/diff-highlight/DiffHighlight.pm
> @@ -4,6 +4,11 @@ use 5.008;
>  use warnings FATAL => 'all';
>  use strict;
>  
> +# Use the correct value for both UNIX and Windows (/dev/null vs nul)
> +use File::Spec;
> +
> +my $NULL = File::Spec->devnull();
> +
>  # Highlight by reversing foreground and background. You could do
>  # other things like bold or underline if you prefer.
>  my @OLD_HIGHLIGHT = (
> @@ -134,7 +139,7 @@ sub highlight_stdin {
>  # fallback, which means we will work even if git can't be run.
>  sub color_config {
>   my ($key, $default) = @_;
> - my $s = `git config --get-color $key 2>/dev/null`;
> + my $s = `git config --get-color $key 2>$NULL`;
>   return length($s) ? $s : $default;
>  }
>  
> -- 
> gitgitgadget
>

Regression in rebase-in-C with rebase.autoStash=true

On Tue, Oct 23 2018, Johannes Schindelin via GitGitGadget wrote:

> Johannes Schindelin (2):
>   rebase --autostash: demonstrate a problem with dirty submodules
>   rebase --autostash: fix issue with dirty submodules
>
>  builtin/rebase.c|  2 +-
>  t/t3420-rebase-autostash.sh | 10 ++
>  2 files changed, 11 insertions(+), 1 deletion(-)

There's another bug with rebase.autoStash in master (and next/pu) but
not v2.19.0. I tried to bisect bit it just comes down to 5541bd5b8f
("rebase: default to using the builtin rebase", 2018-08-08).

Credit to a co-worker of mine who wishes to remain anonymous for
discovering this. I narrowed down his test-case to (any repo will do):

(
rm -rf /tmp/todo &&
git clone --single-branch --no-tags --branch=todo 
https://github.com/git/git.git /tmp/todo &&
cd /tmp/todo &&
rm Make &&
git rev-parse --abbrev-ref HEAD &&
git -c rebase.autoStash=true -c pull.rebase=true pull &&
if test $(git rev-parse --abbrev-ref HEAD) != 'todo'
then
echo 'On detached head!' &&
git status &&
exit 1
else
echo 'We are still on our todo branch!'
fi
)

Checkout deleted semi-untracked file

2018-11-06 Thread Steffen Jost


Hello!

A brief discussion on the git user mailing list on Google Groups recommended me 
to file the following as a bug report.

The problem led to an actual file loss, but I suspect that this might be 
intended:

1) .gitignore is added to the repository (which then causes problems)
2) A file is added, repeatedly edited and comitted to a remote repository.
3) Later on, the file is added to .gitignore and "git rm --cached file" is 
executed (since the file now contains information private to each developer).
4) Several commits happen.
5) I checkout an old branch which has not yet seen the change in .gitignore in 
the master branch. The file is reverted to the state of the branch.
6) I checkout master, and the file with all later changes is irrevocably lost.

I usually advise my students to check-in their .gitignore file into the 
repository. Apparently this is a bad advice, since it now led to a somewhat 
painful file loss for me.

So what is the actual advice on this? Google turned up mixed advice there, and 
the git user mailing list on Google Groups recommended me submitting this as a 
bug here. However, I think this works as intended. However, I don't know either 
how to avert this problem, apart from not checking in the .gitignore file (or 
checking it in under a different name and copying it manually).


Thanks for any advice,
 Steffen Jost.

--
+49-89-2180-9139
http://www.tcs.ifi.lmu.de/~jost/

Lehr- und Forschungseinheit für Theoretische Informatik
Ludwig-Maximilians-Universität München
Oettingenstr. 67 (E111)
80538 München
BAVARIA

What exactly is a "initial checkout"

2018-11-06 Thread Christian Halstrick

I am trying to teach JGit [1] to behave like native git regarding some
corner cases during "git checkout". I am reading the "git read-tree"
documentation and I am not sure about the case [2]. Git should behave
differently during a normal checkout than when you are doing a
"initial checkout". I can imagine that the first checkout you do after
you have cloned a repo is a initial checkout but: What exactly defines
a "initial checkout"? It can't be an empty or non-existing index
because native git behaves like in a non-initial-checkout even if the
index is empty (see example below).

Here are some commands explaining my case. Git is facing an empty
index, HEAD and MERGE (the commit you checkout) have the some content
for path 'p'  and still git is neither updating index nor workingtree
file during checkout.

git init
mkdir p
echo initial >p/a
git add p/a
git commit -m initial
touch p2
git add p2
git commit -m followup
git rm -r p p2
echo "important data" >p
git checkout HEAD~ # successful checkout leaving p dirty
cat p # prints "important data", so 'p' is not updated during the checkout
git ls-files -sv  # empty -> index is empty

[1] https://www.eclipse.org/jgit/
[2] https://github.com/git/git/blob/master/Documentation/git-read-tree.txt#L187

Change your password 06239 immediately. Your account has been hacked.

2018-11-06 Thread git

I greet you!

I have bad news for you.
27/08/2018 - on this day I hacked your operating system and got full access to 
your account git@vger.kernel.org
On that day your account (git@vger.kernel.org) password was: 06239

It is useless to change the password, my malware intercepts it every time.

How it was:
In the software of the router to which you were connected that day, there was a 
vulnerability.
I first hacked this router and placed my malicious code on it.
When you entered in the Internet, my trojan was installed on the operating 
system of your device.

After that, I made a full dump of your disk (I have all your address book, 
history of viewing sites, all files, phone numbers and addresses of all your 
contacts).

A month ago, I wanted to lock your device and ask for a small amount of money 
to unlock.
But I looked at the sites that you regularly visit, and came to the big delight 
of your favorite resources.
I'm talking about sites for adults.

I want to say - you are a big pervert. You have unbridled fantasy!

After that, an idea came to my mind.
I made a screenshot of the intimate website where you have fun (you know what 
it is about, right?).
After that, I took off your joys (using the camera of your device). It turned 
out beautifully, do not hesitate.

I am strongly belive that you would not like to show these pictures to your 
relatives, friends or colleagues.
I think $925 is a very small amount for my silence.
Besides, I spent a lot of time on you!

I accept money only in Bitcoins.
My BTC wallet: 12ziVv4aQkZTA1gj86Y9uYQByG4CcdVcTA

You do not know how to replenish a Bitcoin wallet?
In any search engine write "how to send money to btc wallet".
It's easier than send money to a credit card!

For payment you have a little more than two days (exactly 50 hours).
Do not worry, the timer will start at the moment when you open this letter. 
Yes, yes .. it has already started!

After payment, my virus and dirty photos with you self-destruct automatically.
Narrative, if I do not receive the specified amount from you, then your device 
will be blocked, and all your contacts will receive a photos with your "joys".

I want you to be prudent.
- Do not try to find and destroy my virus! (All your data is already uploaded 
to a remote server)
- Do not try to contact me (this is not feasible, I sent you an email from your 
account)
- Various security services will not help you; formatting a disk or destroying 
a device will not help either, since your data is already on a remote server.

P.S. I guarantee you that I will not disturb you again after payment, as you 
are not my single victim.
 This is a hacker code of honor.

>From now on, I advise you to use good antiviruses and update them regularly 
>(several times a day)!

Don't be mad at me, everyone has their own work.
Farewell.

RE

2018-11-06 Thread Alice Waltoni

I have a charity mission worth $ 100,000,000.00 from you contact me for more 
info

[PATCH] format-patch: respect --stat when explicitly specified

2018-11-06 Thread Leif Lindholm

Commit 43662b23abbd
("format-patch: keep cover-letter diffstat wrapped in 72 columns") made
format-patch keep the diffstat to within 72 characters. However, it does
this even when --stat is explicitly set on the command line.

Make it possible to explicitly override the new mechanism, using --stat,
matching the functionality before this change. This also matches the
output in the case of non-cover-letter files.

Cc: Nguyễn Thái Ngọc Duy 
Cc: Junio C Hamano 
Reported-by: Laszlo Ersek 
Signed-off-by: Leif Lindholm 
---

Note:
In TianoCore we have LotsOfGloriousFilesNamedInReallyLongCamelCase, so
our official submission guidelines specify the use of --stat.

 builtin/log.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 061d4fd86..07e6ae2c1 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,
 
memcpy(, >diffopt, sizeof(opts));
opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
-   opts.stat_width = MAIL_DEFAULT_WRAP;
+   if (rev->diffopt.stat_width == -1)
+   opts.stat_width = MAIL_DEFAULT_WRAP;
 
diff_setup_done();
 
-- 
2.11.0

Re: [PATCH] range-diff: add a --no-patch option to show a summary

2018-11-06 Thread Johannes Schindelin

Hi,

On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> On Mon, Nov 05 2018, Eric Sunshine wrote:
>
> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason  
> > wrote:
> >> Add a --no-patch option which shows which changes got removed, added
> >> or moved etc., without showing the diff associated with them.
> >
> > This option existed in the very first version[1] of range-diff (then
> > called branch-diff) implemented by Dscho, although it was called
> > --no-patches (with an "es"), which it inherited from tbdiff. I think
> > someone (possibly me) pointed out that --no-patch (sans "es") would be
> > more consistent with existing Git options. I don't recall why Dscho
> > removed the option during the re-rolls, but the explanation may be in
> > that thread.
>
> Thanks for digging. Big thread, not going to re-read it now. I'd just
> like to have this.

In my hands, the well-documented `-s` option works (see e.g.
https://git-scm.com/docs/git-diff#git-diff--s), although I have to admit
that the `git-range-diff` manual does not talk about the diff-options.

And for the record, for me, `git range-diff A...B --no-patch` *already*
works.

Ciao,
Dscho

>
> > I was also wondering if --summarize or --summary-only might be a
> > better name, describing the behavior at a higher level, but since
> > there is precedent for --no-patch (or --no-patches in tbdiff), perhaps
> > the name is fine as is.
>
> I think we should aim to keep a 1=1 mapping between range-diff and
> log/show options when possible, even though the output might have a
> slightly different flavor as my 4th paragraph discussing a potential
> --stat talks about.
>
> E.g. I can imagine that range-diff --no-patch --stat --summary would not
> show the patch, but a stat as described there, plus e.g. a "create
> mode..." if applicable.
>
> This change implements only a tiny fraction of that, but it would be
> very neat if we supported more stuff, and showed it in range-diff-y way,
> e.g. some compact format showing:
>
> 1 file changed, 3->2 insertions(+), 10->9 deletions(-)
> create mode 100(6 -> 7)44 new-executable
>
> > The patch itself looks okay.
> >
> > [1]: 
> > https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schinde...@gmx.de/
>

Bug: git-svn clone --preserve-empty-dirs fail - couldn't truncate file at /usr/share/perl5/Git.pm line 1337.

2018-11-06 Thread Georg Tsakumagos

Hi, i am in the middle of a migration project. Almost over 60 projects 
(aprox 1/3) suffer from this behaviour. I was able to create a svn 
repository dump (8MB) of a small project thats provoke that failure. The 
failure do not occur if the option "--preserve-empty-dirs" is omitted. 
Also no problem if the svn dump is filtered with "svndumpfilter 
--drop-empty-revs" before load into a fresh repo.


I'll be able to provide the dump for further investigations. I would 
send it on demand. I'm able to reproduce the failure with the dump. It 
seemed many others had suffered from this bug.


Used Version:   1:2.17.1-1ubuntu0.3
OS:                       Ubuntu 16.04.5 LTS
Commandline: /usr/bin/git svn clone --prefix= --preserve-empty-dirs 
--authors-file=/mnt/migration/authors.txt --stdlayout https://host/repo 
/mnt/migration/repos/TEST/plausiweb


r37895 = bf620e85ad1ac0d42298cb01cb0dab594958d1e1 (refs/remotes/trunk)
    A   release-pom.xml
    A   plausiweb-report/release-pom.xml
    M   plausiweb-report/pom.xml
    A   plausiweb-web/release-pom.xml
    M   plausiweb-web/pom.xml
    M   pom.xml
r37896 = 9420d7081b44797825aa7a19bb508ef2533a3356 (refs/remotes/trunk)
Found possible branch point: https://scm/svn/git/java/plausiweb/trunk => 
https://scm/svn/git/java/plausiweb/tags/plausiweb-18.06.1, 37896
Found branch parent: (refs/remotes/tags/plausiweb-18.06.1) 
9420d7081b44797825aa7a19bb508ef2533a3356

Following parent with do_switch
couldn't truncate file at /usr/share/perl5/Git.pm line 1337.


--

Best regards from Bremen

Georg Tsakumagos

build error on mac os 10.14.1

2018-11-06 Thread yan ke

Hello

when build on mac os 10.14.1 with the master branch, I got the
error as blew, so what is wrong?

ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
clangclang: error: linker command failed with exit code 1 (use -v to
see invocation)
: error: linker command failed with exit code 1 (use -v to see invocation)
clang: error: linker command failed with exit code 1 (use -v to see invocation)
clangmake: *** [Makefile:2369: git-shell] Error 1
make: *** Waiting for unfinished jobs
make: *** [Makefile:2369: git-sh-i18n--envsubst] Error 1
: error: linker command failed with exit code 1 (use -v to see invocation)
clang: make: *** [Makefile:2369: git-credential-cache--daemon] Error 1
error: linker command failed with exit code 1 (use -v to see invocation)
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:2369: git-credential-cache] Error 1
make: *** [Makefile:2369: git-credential-store] Error 1
make: *** [Makefile:2383: git-remote-testsvn] Error 1
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:2393: git-remote-http] Error 1
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:2369: git-http-backend] Error 1
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:2372: git-imap-send] Error 1
make: *** [Makefile:2379: git-http-push] Error 1
clang: error: linker command failed with exit code 1 (use -v to see invocation)
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:2376: git-http-fetch] Error 1
make: *** [Makefile:2369: git-daemon] Error 1
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:2369: git-fast-import] Error 1
ld: archive has no table of contents file 'xdiff/lib.a' for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:2046: git] Error 1

Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

On Sun, Nov 04 2018, Nguyễn Thái Ngọc Duy wrote:

> When a commit is reverted (or cherry-picked with -x) we add an English
> sentence recording that commit id in the new commit message. Make
> these real trailer lines instead so that they are more friendly to
> parsers (especially "git interpret-trailers").
>
> A reverted commit will have a new trailer
>
> Revert: 
>
> Similarly a cherry-picked commit with -x will have
>
> Cherry-Pick: 
> [...]
>  I think standardizing how we record commit ids in the commit message
>  is a good idea. Though to be honest this started because of my irk of
>  an English string "cherry picked from..." that cannot be translated.
>  It might as well be a computer language that happens to look like
>  English.
> [...]
> @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   base_label = msg.label;
>   next = parent;
>   next_label = msg.parent_label;
> - strbuf_addstr(, "Revert \"");
> - strbuf_addstr(, msg.subject);
> - strbuf_addstr(, "\"\n\nThis reverts commit ");
> - strbuf_addstr(, oid_to_hex(>object.oid));
> -
> - if (commit->parents && commit->parents->next) {
> - strbuf_addstr(, ", reversing\nchanges made to ");
> - strbuf_addstr(, oid_to_hex(>object.oid));
> - }
> - strbuf_addstr(, ".\n");
> + strbuf_addf(, "Revert \"%s\"\n\n", msg.subject);
> +
> + strbuf_addf(, "Revert: %s\n",
> + oid_to_hex(>object.oid));
>   } else {
>   const char *p;

Others have already commented on the backwards-compatibility /
switchover concerns, so I won't spend much time on that. Except to say
that I don't think changing this would be a big deal.

Anyone trying to parse out /This reverts commit/ or other pre-set
English texts we put into the commit object is already needing to deal
with users changing the message. E.g. I have a habit of doing partial
reverts and changing it to "This partially reverts..." etc.

Leaving aside the question of whether the pain of switching is worth it,
I think it's a worthwihle to consider if we could stop hardcoding one
specific human language in commit messages, and instead leave something
machine-readable behind.

We do that with reverts, and also with merge commits, which could be
given a similar treatment where we change e.g. "Merge branches
'jc/convert', 'jc/bigfile' and 'jc/replacing' into jc/streaming" (to use
git.git's 02071b27f1 as an example) to:

Merge-branch-1: jc/convert
Merge-branch-2: jc/bigfile
Merge-branch-3: jc/replacing
Merge-branch-into: jc/streaming

Then, when rendering the commit in the UI we could parse that out, and
put a "Merge branches[...]" message at the top, except this time in the
user's own language.

Re: [PATCH] range-diff: add a --no-patch option to show a summary