Re: [PATCH v3] daemon: add --log-destination=(stderr|syslog|none)

[Eric Sunshine]

On Sat, Feb 3, 2018 at 6:08 PM, Lucas Werkmeister
 wrote:
> This new option can be used to override the implicit --syslog of
> --inetd, or to disable all logging. (While --detach also implies
> --syslog, --log-destination=stderr with --detach is useless since
> --detach disassociates the process from the original stderr.) --syslog
> is retained as an alias for --log-destination=syslog.
> [...]
> Signed-off-by: Lucas Werkmeister 

Thanks for the re-roll. There are a few comments below. Except for one
apparent bug, I'm not sure the others are worth a re-roll...

> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> @@ -110,8 +112,26 @@ OPTIONS
> +--log-destination=::
> +   Send log messages to the specified destination.
> +   Note that this option does not imply --verbose,
> +   thus by default only error conditions will be logged.
> +   The  defaults to `stderr`, and must be one of:

I wonder if this should say instead:

The default destination is `stderr` unless `syslog`
is implied by `--inetd` or `--detach`, ...

> diff --git a/daemon.c b/daemon.c
> @@ -9,7 +9,12 @@
> -static int log_syslog;
> +static enum log_destination {
> +   LOG_DESTINATION_UNSET = -1,
> +   LOG_DESTINATION_NONE = 0,
> +   LOG_DESTINATION_STDERR = 1,
> +   LOG_DESTINATION_SYSLOG = 2,
> +} log_destination;

Doesn't log_destination need to be initialized to
LOG_DESTINATION_UNSET (see [1])? As it stands, being static, it's
initialized automatically to 0 (LOG_DESTINATION_NONE), which borks the
logic below.

> @@ -74,11 +80,14 @@ static const char *get_ip_address(struct hostinfo *hi)
>  static void logreport(int priority, const char *err, va_list params)
>  {
> +   switch (log_destination) {
> +   case LOG_DESTINATION_SYSLOG: {
> char buf[1024];
> vsnprintf(buf, sizeof(buf), err, params);
> syslog(priority, "%s", buf);
> +   break;
> +   }
> +   case LOG_DESTINATION_STDERR:
> /*
>  * Since stderr is set to buffered mode, the
>  * logging of different processes will not overlap
> @@ -88,6 +97,10 @@ static void logreport(int priority, const char *err, 
> va_list params)
> vfprintf(stderr, err, params);
> fputc('\n', stderr);
> fflush(stderr);
> +   break;
> +   case LOG_DESTINATION_NONE:
> +   case LOG_DESTINATION_UNSET:
> +   break;

Since LOG_DESTINATION_UNSET should never occur, perhaps this should be
written as:

case LOG_DESTINATION_NONE:
break;
case LOG_DESTINATION_UNSET:
BUG("impossible destination: %d", log_destination);

> @@ -1297,9 +1309,22 @@ int cmd_main(int argc, const char **argv)
> +   if (skip_prefix(arg, "--log-destination=", )) {
> +   if (!strcmp(v, "syslog")) {
> +   log_destination = LOG_DESTINATION_SYSLOG;
> +   continue;
> +   } else if (!strcmp(v, "stderr")) {
> +   log_destination = LOG_DESTINATION_STDERR;
> +   continue;
> +   } else if (!strcmp(v, "none")) {
> +   log_destination = LOG_DESTINATION_NONE;
> +   continue;
> +   } else
> +   die("Unknown log destination %s", v);

Mentioned previously[1], this probably ought to start with lowercase.
It also would be more readable to set off the unknown value with a
colon or quotes:

die("unknown log destination '%s', v);

> @@ -1402,7 +1426,14 @@ int cmd_main(int argc, const char **argv)
> -   if (log_syslog) {
> +   if (log_destination == LOG_DESTINATION_UNSET) {
> +   if (inetd_mode || detach)
> +   log_destination = LOG_DESTINATION_SYSLOG;
> +   else
> +   log_destination = LOG_DESTINATION_STDERR;
> +   }
> +
> +   if (log_destination == LOG_DESTINATION_SYSLOG) {
> openlog("git-daemon", LOG_PID, LOG_DAEMON);
> set_die_routine(daemon_die);

[1]: 
https://public-inbox.org/git/capig+ctetjq9lsh68fe5otcj9twq9gsbgzdrjzhohtavfvr...@mail.gmail.com/

Re: [PATCH v7 19/31] merge-recursive: add get_directory_renames()

[Eric Sunshine]

On Sat, Feb 3, 2018 at 11:42 PM, Eric Sunshine  wrote:
> [2]: https://en.wikipedia.org/wiki/Diaeresis_(diacritic)

Correction:
[2]: https://en.wikipedia.org/wiki/Precomposed_character

Re: [PATCH v7 19/31] merge-recursive: add get_directory_renames()

[Eric Sunshine]

On Sat, Feb 3, 2018 at 9:04 PM, Elijah Newren  wrote:
> On Sat, Feb 3, 2018 at 2:32 PM, Elijah Newren  wrote:
>> On Fri, Feb 2, 2018 at 5:02 PM, Stefan Beller  wrote:
>>> On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:
 +   while (*--end_of_new == *--end_of_old &&
 +  end_of_old != old_path &&
 +  end_of_new != new_path)
 +   ; /* Do nothing; all in the while loop */
>>>
>>> Assuming many repos are UTF8 (including in their paths),
>>> how does this work with display characters longer than one char?
>>> It should be fine as we cut at the slash?
>>
>> Can UTF-8 characters, other than '/', have a byte whose value matches
>> (unsigned char)('/')?  If so, then I'll need to figure out how to do
>> utf-8 character parsing.  Anyone have pointers?
>
> Well, after digging around for a while, I found this claim on the
> Wikipedia page for UTF-8:
>
>   Since ASCII bytes do not occur when encoding non-ASCII code points
> into UTF-8, UTF-8 is safe to use within most programming and document
> languages that interpret certain ASCII characters in a special way,
> such as "/" in filenames, "\" in escape sequences, and "%" in printf.
>
> So, unless I'm reading something wrong here, I think that means this
> code is just fine as it is.

You're reading it correctly. Unicode values greater than \U007f
encoded with UTF-8 will never contain bytes which can be confused with
any 7-bit ASCII character.

It's possible that Stefan was thinking of "combining characters"[1]
which may be "precomposed" and "decomposed"[2], but which appear the
same when rendered. For instance, "ö" might be a single Unicode
codepoint or two codepoints, such as "o" combined with a diaeresis.
It's a potential problem if you're comparing, byte by byte, two
filenames which look the same. However, Git takes pains[3] to avoid
this problem by ensuring (if possible) that filenames are precomposed
within Git even if they happen to be decomposed on the actual
filesystem. So, most likely, your code is okay as-is.

[1]: https://en.wikipedia.org/wiki/Combining_character
[2]: https://en.wikipedia.org/wiki/Diaeresis_(diacritic)
[3]: https://github.com/git/git/blob/master/compat/precompose_utf8.c

PLEASE I NEED YOUR HELP FOR MY EDUCATION

[IBRAHIM COULIBALY]

My Dear Father,

Good day !

Please permit me to introduce myself, I am MIMI IBRAHIM COULIBALY 17
years old female from the Republic of Ivory Coast,  in Abidjan; I'm
the Daughter of Late Chief Sgt. Ibrahim Coulibaly (A.K.A General
Warlord IB ). My late Father was a well known Ivory Coast military
leader. He died on Thursday 28 April 2011 following a fight with the
Republican Forces of Ivory Coast (FRCI).

You can read more about my late father in the news graduating of ivory
cost  in bouake.

www.guardian.co.uk/world/2011/apr/28/ivory-coast-renegade-warlord-ibrahim-coulibaly

Please I want to leave my situation here to come over to your country
to continue my education.

I have made research before contacting you and please I want you to
help me come to your country to start my new life.
I will tell you more about my condition if you are willing to help me
come over to your country to continue my education.
I will truly appreciate your help to my life for my education.
I will be happy to hear from you
Miss. MIMI IBRAHIM COULIBALY

Re: [PATCH v7 19/31] merge-recursive: add get_directory_renames()

[Elijah Newren]

On Sat, Feb 3, 2018 at 2:32 PM, Elijah Newren  wrote:
> On Fri, Feb 2, 2018 at 5:02 PM, Stefan Beller  wrote:
>> On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:

>>> +   while (*--end_of_new == *--end_of_old &&
>>> +  end_of_old != old_path &&
>>> +  end_of_new != new_path)
>>> +   ; /* Do nothing; all in the while loop */
>>
>> We have to compare manually as we'd want to find
>> the first non-equal and there doesn't seem to be a good
>> library function for that.
>>
>> Assuming many repos are UTF8 (including in their paths),
>> how does this work with display characters longer than one char?
>> It should be fine as we cut at the slash?
>
> Oh, UTF-8.  Ugh.
> Can UTF-8 characters, other than '/', have a byte whose value matches
> (unsigned char)('/')?  If so, then I'll need to figure out how to do
> utf-8 character parsing.  Anyone have pointers?

Well, after digging around for a while, I found this claim on the
Wikipedia page for UTF-8:

  Since ASCII bytes do not occur when encoding non-ASCII code points
into UTF-8, UTF-8 is safe to use within most programming and document
languages that interpret certain ASCII characters in a special way,
such as "/" in filenames, "\" in escape sequences, and "%" in printf.

So, unless I'm reading something wrong here, I think that means this
code is just fine as it is.

[GSoC][PATCH] commit: add a commit.signOff config variable

[Chen Jingpiao]

Add the commit.signOff configuration variable to use the -s or --signoff
option of git commit by default.

Signed-off-by: Chen Jingpiao 
---

Though we can configure signoff using format.signOff variable. Someone like to
add Signed-off-by line by the committer.

 Documentation/config.txt |  4 +++
 Documentation/git-commit.txt |  2 ++
 builtin/commit.c |  4 +++
 t/t7501-commit.sh| 69 
 4 files changed, 79 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0e25b2c92..5dec3f0cb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1303,6 +1303,10 @@ commit.gpgSign::
convenient to use an agent to avoid typing your GPG passphrase
several times.
 
+commit.signOff::
+   A boolean value which lets you enable the `-s/--signoff` option of
+   `git commit` by default. See linkgit:git-commit[1].
+
 commit.status::
A boolean to enable/disable inclusion of status information in the
commit message template when using an editor to prepare the commit
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f970a4342..7a28ea765 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -166,6 +166,8 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and 
`-F`.
the rights to submit this work under the same license and
agrees to a Developer Certificate of Origin
(see http://developercertificate.org/ for more information).
+   See the `commit.signOff` configuration variable in
+   linkgit:git-config[1].
 
 -n::
 --no-verify::
diff --git a/builtin/commit.c b/builtin/commit.c
index 4610e3d8e..324213254 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1548,6 +1548,10 @@ static int git_commit_config(const char *k, const char 
*v, void *cb)
sign_commit = git_config_bool(k, v) ? "" : NULL;
return 0;
}
+   if (!strcmp(k, "commit.signoff")) {
+   signoff = git_config_bool(k, v);
+   return 0;
+   }
if (!strcmp(k, "commit.verbose")) {
int is_bool;
config_commit_verbose = git_config_bool_or_int(k, v, _bool);
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index fa61b1a4e..46733ed2a 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -505,6 +505,75 @@ Myfooter: x" &&
test_cmp expected actual
 '
 
+test_expect_success "commit.signoff=true and --signoff omitted" '
+   echo 7 >positive &&
+   git add positive &&
+   git -c commit.signoff=true commit -m "thank you" &&
+   git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+   (
+   echo thank you
+   echo
+   git var GIT_COMMITTER_IDENT |
+   sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
+   ) >expected &&
+   test_cmp expected actual
+'
+
+test_expect_success "commit.signoff=true and --signoff" '
+   echo 8 >positive &&
+   git add positive &&
+   git -c commit.signoff=true commit --signoff -m "thank you" &&
+   git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+   (
+   echo thank you
+   echo
+   git var GIT_COMMITTER_IDENT |
+   sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
+   ) >expected &&
+   test_cmp expected actual
+'
+
+test_expect_success "commit.signoff=true and --no-signoff" '
+   echo 9 >positive &&
+   git add positive &&
+   git -c commit.signoff=true commit --no-signoff -m "thank you" &&
+   git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+   echo thank you >expected &&
+   test_cmp expected actual
+'
+
+test_expect_success "commit.signoff=false and --signoff omitted" '
+   echo 10 >positive &&
+   git add positive &&
+   git -c commit.signoff=false commit -m "thank you" &&
+   git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+   echo thank you >expected &&
+   test_cmp expected actual
+'
+
+test_expect_success "commit.signoff=false and --signoff" '
+   echo 11 >positive &&
+   git add positive &&
+   git -c commit.signoff=false commit --signoff -m "thank you" &&
+   git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+   (
+   echo thank you
+   echo
+   git var GIT_COMMITTER_IDENT |
+   sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
+   ) >expected &&
+   test_cmp expected actual
+'
+
+test_expect_success "commit.signoff=false and --no-signoff" '
+   echo 12 >positive &&
+   git add positive &&
+   git -c commit.signoff=false commit --no-signoff -m "thank you" &&
+   git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+   echo thank you >expected &&
+   test_cmp expected actual
+'
+
 test_expect_success 'multiple -m' '
 
>negative &&
-- 

our urgent respond immediately

[Samira Mohamed]

Hi Friend I am a banker in UBA BANK .I want to transfer an abandoned sum of 
10.5 millions USD to your account.50% will be for you. No risk involved. 
Contact me for more details. Kindly reply me back to my alternative email 
address ( samiramohamed5...@gmail.com) mrs samira mohamed

[PATCH v3] daemon: add --log-destination=(stderr|syslog|none)

[Lucas Werkmeister]

This new option can be used to override the implicit --syslog of
--inetd, or to disable all logging. (While --detach also implies
--syslog, --log-destination=stderr with --detach is useless since
--detach disassociates the process from the original stderr.) --syslog
is retained as an alias for --log-destination=syslog.

--log-destination always overrides implicit --syslog regardless of
option order. This is different than the “last one wins” logic that
applies to some implicit options elsewhere in Git, but should hopefully
be less confusing. (I also don’t know if *all* implicit options in Git
follow “last one wins”.)

The combination of --inetd with --log-destination=stderr is useful, for
instance, when running `git daemon` as an instanced systemd service
(with associated socket unit). In this case, log messages sent via
syslog are received by the journal daemon, but run the risk of being
processed at a time when the `git daemon` process has already exited
(especially if the process was very short-lived, e.g. due to client
error), so that the journal daemon can no longer read its cgroup and
attach the message to the correct systemd unit (see systemd/systemd#2913
[1]). Logging to stderr instead can solve this problem, because systemd
can connect stderr directly to the journal daemon, which then already
knows which unit is associated with this stream.

[1]: https://github.com/systemd/systemd/issues/2913

Helped-by: Ævar Arnfjörð Bjarmason 
Helped-by: Junio C Hamano 
Helped-by: Eric Sunshine 
Signed-off-by: Lucas Werkmeister 
---

Notes:
Next version on my quest to make git-daemon logging more reliable :)
many thanks to Eric Sunshine for review. Main changes this version:

- Rename --send-log-to to --log-destination, following the
  postgresql option. A cursory search didn’t turn up any other
  daemons with a similar option – suggestions welcome!
- Make explicit --log-destination always override implicit --syslog,
  regardless of option order.

 Documentation/git-daemon.txt | 26 ++---
 daemon.c | 45 +---
 2 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 3c91db7be..a0106e6aa 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -20,6 +20,7 @@ SYNOPSIS
 [--inetd |
  [--listen=] [--port=]
  [--user= [--group=]]]
+[--log-destination=(stderr|syslog|none)]
 [...]
 
 DESCRIPTION
@@ -80,7 +81,8 @@ OPTIONS
do not have the 'git-daemon-export-ok' file.
 
 --inetd::
-   Have the server run as an inetd service. Implies --syslog.
+   Have the server run as an inetd service. Implies --syslog (may be
+   overridden with `--log-destination=`).
Incompatible with --detach, --port, --listen, --user and --group
options.
 
@@ -110,8 +112,26 @@ OPTIONS
zero for no limit.
 
 --syslog::
-   Log to syslog instead of stderr. Note that this option does not imply
-   --verbose, thus by default only error conditions will be logged.
+   Short for `--log-destination=syslog`.
+
+--log-destination=::
+   Send log messages to the specified destination.
+   Note that this option does not imply --verbose,
+   thus by default only error conditions will be logged.
+   The  defaults to `stderr`, and must be one of:
++
+--
+stderr::
+   Write to standard error.
+   Note that if `--detach` is specified,
+   the process disconnects from the real standard error,
+   making this destination effectively equivalent to `none`.
+syslog::
+   Write to syslog, using the `git-daemon` identifier.
+none::
+   Disable all logging.
+--
++
 
 --user-path::
 --user-path=::
diff --git a/daemon.c b/daemon.c
index e37e343d0..f28400e3f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -9,7 +9,12 @@
 #define initgroups(x, y) (0) /* nothing */
 #endif
 
-static int log_syslog;
+static enum log_destination {
+   LOG_DESTINATION_UNSET = -1,
+   LOG_DESTINATION_NONE = 0,
+   LOG_DESTINATION_STDERR = 1,
+   LOG_DESTINATION_SYSLOG = 2,
+} log_destination;
 static int verbose;
 static int reuseaddr;
 static int informative_errors;
@@ -25,6 +30,7 @@ static const char daemon_usage[] =
 "   [--access-hook=]\n"
 "   [--inetd | [--listen=] [--port=]\n"
 "  [--detach] [--user= [--group=]]\n"
+"   [--log-destination=(stderr|syslog|none)]\n"
 "   [...]";
 
 /* List of acceptable pathname prefixes */
@@ -74,11 +80,14 @@ static const char *get_ip_address(struct hostinfo *hi)
 
 static void logreport(int priority, const char *err, va_list params)
 {
-   if (log_syslog) {
+   switch (log_destination) {
+   case LOG_DESTINATION_SYSLOG: {
char 

Re: [PATCH v7 19/31] merge-recursive: add get_directory_renames()

[Elijah Newren]

On Fri, Feb 2, 2018 at 5:02 PM, Stefan Beller  wrote:
> On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:

>> +   /* For
>
> comment style.

Fixed it and looked through the file for any other violations and
fixed the ones introduced in this series.  (The ones I added years ago
in other places of the file I just left.)

>> +*"a/b/c/d/foo.c" -> "a/b/something-else/d/foo.c"
>> +* the "d/foo.c" part is the same, we just want to know that
>> +*"a/b/c" was renamed to "a/b/something-else"
>> +* so, for this example, this function returns "a/b/c" in
>> +* *old_dir and "a/b/something-else" in *new_dir.
>
> So we would not see multi-directory renames?
>
> "a/b/c/d/foo.c" -> "a/b/something-else/e/foo.c"
>
> could be detected as
>
> "a/b/{c/d/ => something-else/e}/foo.c"
>
> I assume this patch series is not bringing that to the table.
> (which is fine, I am just wondering)

I fully intended to support that, and believe the code does.  I
changed the comment as follows to try to make it clearer:

/*
 * For
 *"a/b/c/d/e/foo.c" -> "a/b/some/thing/else/e/foo.c"
 * the "e/foo.c" part is the same, we just want to know that
 *"a/b/c/d" was renamed to "a/b/some/thing/else"
 * so, for this example, this function returns "a/b/c/d" in
 * *old_dir and "a/b/some/thing/else" in *new_dir.
 *
 * Also, if the basename of the file changed, we don't care.  We
 * want to know which portion of the directory, if any, changed.
 */

Is that better?

>> +*
>> +* Also, if the basename of the file changed, we don't care.  We
>> +* want to know which portion of the directory, if any, changed.
>> +*/
>> +   end_of_old = strrchr(old_path, '/');
>> +   end_of_new = strrchr(new_path, '/');
>> +
>> +   if (end_of_old == NULL || end_of_new == NULL)
>> +   return;
>
> return early as the files are in the top level, and apparently we do
> not support top level renaming?
>
> What about a commit like 81b50f3ce4 (Move 'builtin-*' into
> a 'builtin/' subdirectory, 2010-02-22) ?
>
> Well that specific commit left many files outside the new builtin dir,
> but conceptually we could see a directory rename of
>
> /* => /src/*

We had some internal usecases for want to support a "rename" of the
toplevel directory into a subdirectory of itself.  However, attempting
to support that opened much too big a can of worms for me.  We'd open
up some big surprises somewhere.

In particular, note that not supporting a "rename" of the toplevel
directory is a special case of not supporting a "rename" of any
directory to a subdirectory below itself, which in turn is a very
special case of excluding partial directory renames.  I addressed this
in the cover letter of my original submission, as follows:

"""
Further, there's a basic question about when directory rename detection
should be applied at all.  I have a simple rule:

  3) If a given directory still exists on both sides of a merge, we do
 not consider it to have been renamed.

Rule 3 may sound obvious at first, but it will probably arise as a
question for some users -- what if someone "mostly" moved a directory but
still left some files around, or, equivalently (from the perspective of the
three-way merge that merge-recursive performs), fully renamed a directory
in one commmit and then recreated that directory in a later commit adding
some new files and then tried to merge?  See the big comment in section 4
of the new t6043 for further discussion of this rule.
"""

Patch 04/31 is the one that adds that big comment with further discussion.

Maybe there's a way to support toplevel renames, but I think it'd make
this series longer and more complicated...and may cause more edge
cases that confuse users.

>> +   while (*--end_of_new == *--end_of_old &&
>> +  end_of_old != old_path &&
>> +  end_of_new != new_path)
>> +   ; /* Do nothing; all in the while loop */
>
> We have to compare manually as we'd want to find
> the first non-equal and there doesn't seem to be a good
> library function for that.
>
> Assuming many repos are UTF8 (including in their paths),
> how does this work with display characters longer than one char?
> It should be fine as we cut at the slash?

Oh, UTF-8.  Ugh.
Can UTF-8 characters, other than '/', have a byte whose value matches
(unsigned char)('/')?  If so, then I'll need to figure out how to do
utf-8 character parsing.  Anyone have pointers?

>> +   /*
>> +* We've found the first non-matching character in the directory
>> +* paths.  That means the current directory we were comparing
>> +* represents the rename.  Move end_of_old and end_of_new back
>> +* to the full directory name.
>> +*/
>> +   if (*end_of_old == '/')
>> +   end_of_old++;
>> +   if (*end_of_old != '/')
>> +

Re: [PATCH v7 17/31] merge-recursive: add a new hashmap for storing directory renames

[Elijah Newren]

On Fri, Feb 2, 2018 at 4:26 PM, Stefan Beller  wrote:
> On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:

>> +static void dir_rename_init(struct hashmap *map)
>> +{
>> +   hashmap_init(map, (hashmap_cmp_fn) dir_rename_cmp, NULL, 0);
>
> See 55c965f3a2 (Merge branch 'sb/hashmap-cleanup', 2017-08-11) or rather
> 201c14e375 (attr.c: drop hashmap_cmp_fn cast, 2017-06-30) for an attempt to
> keep out the casting in the init, but cast the void->type inside the function.

Fixed (locally).

>> +struct dir_rename_entry {
>> +   struct hashmap_entry ent; /* must be the first member! */
>> +   char *dir;
>> +   unsigned non_unique_new_dir:1;
>> +   struct strbuf new_dir;
>> +   struct string_list possible_new_dirs;
>> +};
>
> Could you add comments what these are and if they have any constraints?
> e.g. is 'dir' the full path from the root of the repo and does it end
> with a '/' ?

Good idea, will do.  To help you out with the rest of your review:

yes, dir is full path from the root of the repo, but no it does not
end with a '/'.  The same two constraints hold for how I store
directories in other variables as well.

If someone renames multiple files in a single directory to several
different other directories, then possible_new_dirs is used
(temporarily) to track what those other directories are and how many
files were moved from `dir` to that directory.  The 'winner' (the
target directory with the most renames from `dir` to it) will be
recorded in new_dir.  If there is no 'winner' (a tie for target
directory with the most renames), then non_unique_new_dir is set.
Once we can set either non_unique_new_dir or possible_new_dirs, then
possible_new_dirs is unnecessary and can be freed.

> Having a stringlist of potentially new dirs sounds like the algorithm is
> at least n^2, but how do I know? I'll read on.

Yes, I suppose it's technically n^2, but n is expected to be O(1).
While one can trivially construct a case making n arbitrarily large,
statistically for real world repositories, I expected the mode of n to
be 1 and the mean to be less than 2.  My original idea was to use a
hash for possible_new_dirs, but since hashes are so painful in C and n
should be very small anyway, I didn't bother.  If anyone can find an
example of a real world open source repository (linux, webkit, git,
etc.) with a merge where n is greater than about 10, I'll be
surprised.

Does that address your concern, or does it sound like I'm kicking the
can down the road?  If it's the latter, we can switch it out.

> This patch only adds static functions, so some compilers may even refuse
> to compile after this patch (-Werror -Wunused). I am not sure how strict we
> are there, but as git-bisect still hasn't learned about going only
> into the first
> parent to have bisect working on a "per series" level rather than a "per 
> commit"
> level, it is possible that someone will end up on this commit in the future 
> and
> it won't compile well. :/
>
> Not sure what to recommend, maybe squash this with the patch that makes
> use of these functions?

I'm unsure as well; I can combine it with 19/31 but I thought that
keeping them separate made it slightly easier to review both.

Re: contrib/completion/git-completion.bash: declare -g is not portable

[Jeff King]

On Sat, Feb 03, 2018 at 08:51:16PM +0100, Andreas Schwab wrote:

> On Feb 03 2018, Torsten Bögershausen  wrote:
> 
> > What is "declare -g" good for ?
> 
>   -gcreate global variables when used in a shell function; 
> otherwise
> ignored
> 
> When used in a function, `declare' makes NAMEs local, as with the `local'
> command.  The `-g' option suppresses this behavior.

I think the bigger question is why one would use "declare -g" instead of
just assigning the variable with "var=value".

Glancing at the code, I suspect it is because the name of the variable
itself needs expanded. If that's the case, we could use eval instead,
like:

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 3cc815be0d..204d620ff7 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -297,7 +297,7 @@ __gitcomp_builtin ()
eval "options=\$$var"
 
if [ -z "$options" ]; then
-   declare -g "$var=$(__git ${cmd/_/ } --git-completion-helper)"
+   eval "$var=\$(__git \${cmd/_/ } --git-completion-helper)"
eval "options=\$$var"
fi
 

-Peff

Re: contrib/completion/git-completion.bash: declare -g is not portable

[Andreas Schwab]

On Feb 03 2018, Torsten Bögershausen  wrote:

> What is "declare -g" good for ?

  -gcreate global variables when used in a shell function; otherwise
ignored

When used in a function, `declare' makes NAMEs local, as with the `local'
command.  The `-g' option suppresses this behavior.

Andreas.

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

Re: Segmentation fault in git cherry-pick

[Ayke van Laethem]

> This looks like the stack trace in
> https://github.com/git-for-windows/git/issues/952, which was fixed by
> Johannes Schindelin in commit
> 55e9f0e5c ("merge-recursive: handle NULL in add_cacheinfo()
> correctly", 2016-11-26).  Could you retry with a newer version of git?

A newer version (2.14.2 from Debian stretch-backports) fixes the
issue. I can now do my cherry-pick.
Thank you!

contrib/completion/git-completion.bash: declare -g is not portable

[Torsten Bögershausen]

Hej Duy,
After running t9902-completion.sh on Mac OS I got a failure
in this style:

.../projects/git/git.pu/t/../contrib/completion/git-completion.bash: line 300:
declare: -g: invalid option
declare: usage: declare [-afFirtx] [-p] [name[=value] ...]
--- expected2018-02-03 17:10:18.0 +
+++ out 2018-02-03 17:10:18.0 +
@@ -1,15 +1,2 @@
---quiet
---detach
---track
---orphan=
---ours
---theirs
---merge
---conflict=
---patch
---ignore-skip-worktree-bits
---ignore-other-worktrees
---recurse-submodules
---progress
 --no-track
 --no-recurse-submodules
not ok 92 - double dash "git checkout"

What is "declare -g" good for ?

Assalamu`Alaikum.

[Mohammad ouattara]

Dear Sir/Madam.

Assalamu`Alaikum.

I am Dr mohammad ouattara, I have  ($14.6 Million us dollars) to
transfer into your account,

I will send you more details about this deal and the procedures to
follow when I receive a positive response from you,

Have a great day,
Dr mohammad ouattara.

Re: [PATCH v2 05/14] commit-graph: implement git-commit-graph --write

[Jeff King]

On Fri, Feb 02, 2018 at 08:58:52PM -0500, Derrick Stolee wrote:

> I don't think pairing this with pack-objects or index-pack is a good
> direction, because the commit graph is not locked into a packfile the way
> the bitmap is. In fact, the entire ODB could be replaced independently and
> the graph is still valid (the commits in the graph may no longer have their
> paired commits in the ODB due to a GC; you should never navigate to those
> commits without having a ref pointing to them, so this is not immediately a
> problem).

One advantage of tying this to packs is that you can piggy-back on the
.idx to avoid storing object ids a second time. If we imagine that you
use a 32-bit index into the .idx instead, that's a savings of 16 bytes
per object (or more when we switch to a longer hash). You only need to
refer to commits and their root trees, though. So on something like
linux.git, you're talking about 2 * 700k * 16 = 21 megabytes you could
save.

That may not be worth worrying about too much, compared to the size of
the rest of the data. Disk space is obviously cheap, but I'm more
concerned about working-set size. However, 21 megabytes probably isn't
breaking the bank there these days (and it may even be faster, since the
commit-graph lookups can use the more compact index that contains only
commits, not other objects).

The big advantage of your scheme is that you can update the graph index
without repacking. The traditional advice has been that you should
always do a full repack during a gc (since it gives the most delta
opportunities). So metadata like reachability bitmaps were happy to tied
to packs, since you're repacking anyway during a gc. But my
understanding is that this doesn't really fly with the Windows
repository, where it's simply so big that you never obtain a single
pack, and just pass around slices of history in pack format.

So I think I'm OK with the direction here of keeping metadata caches
separate from the pack storage.

> This sort of interaction with GC is one reason why I did not include the
> automatic updates in this patch. The integration with existing maintenance
> tasks will be worth discussion in its own right. I'd rather demonstrate the
> value of having a graph (even if it is currently maintained manually) and
> then follow up with a focus to integrate with repack, gc, etc.
> 
> I plan to clean up this patch on Monday given the feedback I received the
> last two days (Thanks Jonathan and Szeder!). However, if the current builtin
> design will block merging, then I'll wait until we can find one that works.

If they're not tied to packs, then I think having a separate builtin
like this is the best approach. It gives you a plumbing command to
experiment with, and it can easily be called from git-gc.

-Peff

Re: error: unable to create file: Illegal byte sequence

[Ævar Arnfjörð Bjarmason]

On Sat, Feb 3, 2018 at 4:17 AM, Brian Buchalter  wrote:
> I am attempting to repair a git repo which has an illegal byte
> sequence but am not sure how to proceed. Steps to reproduce: `git
> clone https://github.com/christopherpow/nes-test-roms.git` results in:
>
> ```
> Cloning into 'nes-test-roms'...
> remote: Counting objects: 1049, done.
> remote: Total 1049 (delta 0), reused 0 (delta 0), pack-reused 1049
> Receiving objects: 100% (1049/1049), 5.23 MiB | 8.97 MiB/s, done.
> Resolving deltas: 100% (406/406), done.
> error: unable to create file other/Duelito - L�eme.txt: Illegal byte sequence
> fatal: unable to checkout working tree
> warning: Clone succeeded, but checkout failed.
> You can inspect what was checked out with 'git status'
> and retry the checkout with 'git checkout -f HEAD'
> ```

You can clone my fork of this which fixes the issue:
https://github.com/avar/nes-test-roms

As for fixing this yourself, if your FS can't represent files in the
repo the easiest thing is to clone it on a VM with an OS that can
(e.g. Linux), or alternatively (and harder for non-experts) is to
clone it with --bare and manually create a fixed tree with the various
plumbing commands mentioned in "man git".

New Order

[Lin.]

Dear git@vger.kernel.org

How are you? We are in need of your products/service,

Kindly send us your FOB
and MOQ so that we can place our order.
The order will deliver in Taiwain branch

Looking forward to your respond asap.

Best Regards,
Lin Tou

Prime Success Co. Ltd

Address: Av. Praia Grande,
No.367-371,
Keng Ou Commercial Building, 
17th Fl. C, Macau
Tel: 853-2843-7903