Re: [PATCH 29/59] config.txt: move i18n.* to a separate file

2018-10-21 Thread Andrei Rybak
Subject line: s/i18n/index/ 

On 20/10/2018 14:38, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  Documentation/config.txt   | 11 +--
>  Documentation/index-config.txt | 10 ++
>  2 files changed, 11 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/index-config.txt
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ec0f4e2161..5ba7975837 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -606,16 +606,7 @@ imap::
>   The configuration variables in the 'imap' section are described
>   in linkgit:git-imap-send[1].
>  
> -index.threads::
> - Specifies the number of threads to spawn when loading the index.
> - This is meant to reduce index load time on multiprocessor machines.
> - Specifying 0 or 'true' will cause Git to auto-detect the number of
> - CPU's and set the number of threads accordingly. Specifying 1 or
> - 'false' will disable multithreading. Defaults to 'true'.

[...]


Re: t7005-editor.sh failure

2018-09-26 Thread Andrei Rybak
On 2018-09-26 21:16, Junio C Hamano wrote:
> While at it, update the way to creatd these scripts to use the

s/creatd/create/

Or rewording as "... update the way these scripts are created ..."

> write_script wrapper, so that we do not have to worry about writing
> the she-bang line and making the result executable.


Re: Thank you for public-inbox!

2018-08-29 Thread Andrei Rybak
On 2018-08-29 12:02, Eric Wong wrote:
> Anyways I hope to teach public-inbox to auto-linkify Message-ID-looking
> strings "" into URLs for domain-portability,
> (but it's ambiguous with email addresses).  But yeah, I don't
> like things being tied to domain names.

This would be very useful for people who use MUAs without
Message-ID lookup capabilities, even without domain-portability.


Re: Automatic core.autocrlf?

2018-08-27 Thread Andrei Rybak
On 2018-08-27 19:32, Andrei Rybak wrote:
> 
> How about just using unconditional includes?
> 
> global.gitconfig (synced across machines):
> 
>   [include]
>   path = platform-specific.gitconfig
> 
> And two version of file named "platform-specific.gitconfig", which
> are not synced, and include only code.autocrlf setting.

Robert, in this setup you might also want (just for convenience)
to sync files "platform1.gitconfig" and "platform2.gitconfig", so
that they are available everywhere for editing and can be copied
into local version of platform-specific.gitconfig at any time.



Re: Automatic core.autocrlf?

2018-08-27 Thread Andrei Rybak
On 2018-08-27 17:52, Duy Nguyen wrote:
> On Mon, Aug 27, 2018 at 5:37 PM Torsten Bögershausen  wrote:
>>> In those cases, when it falls back to
>>> configuration for line ending management, I want it to be
>>> automatically configured based on the host platform.
>>
> 
> An alternative is supporting conditional config includes based on
> platform or host name, but I don't know if there are more use cases
> like this to justify it.
> 

How about just using unconditional includes?

global.gitconfig (synced across machines):

  [include]
  path = platform-specific.gitconfig

And two version of file named "platform-specific.gitconfig", which
are not synced, and include only code.autocrlf setting.

--
Best regards, Andrei R.


Re: [PATCH 2/9] introduce hasheq() and oideq()

2018-08-25 Thread Andrei Rybak
On 25/08/18 10:05, Jeff King wrote:
> The main comparison functions we provide for comparing
> object ids are hashcmp() and oidcmp(). These are more
> flexible than a strict equality check, since they also
> express ordering. That makes them them useful for sorting

s/them them/them/

> We can solve that by introducing a hasheq() function (and
> matching oideq() wrapper), which callers can use to make

s/make/& it/

> clear that they only care about equality. For now, the
> implementation will literally be "!hashcmp()", but it frees
> us up later to introduce code optimized specifically for the
> equality check.
> 
> Signed-off-by: Jeff King 



Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

2018-08-19 Thread Andrei Rybak
On 19/08/18 22:32, Jeff King wrote:
> On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:
> 
>>   1. Check both files at the same time (combination with Gábor's
>>  function):
>>
>>  test_cmp () {
>>  if test "$1" != - &&
>> test "$2" != - &&
>> ! test -s "$1" && 
>> ! test -s "$2"
>>  then
>>  error "bug in test script: using test_cmp to check 
>> empty file; use test_must_be_empty instead"
>>  fi
>>  test_cmp_allow_empty "$@"
>>  }
>>
>>  This will still be reporting to the developer clearly, but
>>  will only catch cases exactly like the bogus test in t5310.
> 
> Doesn't that have the opposite issue? If we expect non-empty output but
> the command produces empty output, we'd say "bug in the test script".
> But that is not true at all; it's a failed test.

No. Only when both "$1" and "$2" are empty files will the function above
report "bug in test script". From patch's commit message:

  ... both invocations produce empty 'pack{a,b}.objects' files, and the
  subsequent 'test_cmp' happily finds those two empty files identical.

That's what I meant by "will only catch cases exactly like the bogus
test in t5310".

However ...

> If we assume that "expect" is first (which is our convention but not
> necessarily guaranteed), then I think the best logic is something like:
> 
>   if $1 is empty; then
> bug in the test script
>   elif test_cmp_allow_empty "$@"
> test failure
> 
> We do not need to check $2 at all. An empty one is either irrelevant (if
> the expectation is empty), or a test failure (because it would not match
> the non-empty $1).

... this is indeed a better solution. I written out the cases for
updated test_cmp to straighten out my thinking:

 * both $1 and $2 are empty:
 bogus test:
   needs either fixing generation of both expect and actual
   or switching to test_must_be_empty
   OR
 bogus helper function, as Gábor described above:
   needs to switch to test_cmp_allow_empty
 
 * $1 is non-empty && $2 is empty
   proceeding with test
   test failure from GIT_TEST_CMP
 
 * $1 is empty && $2 is non-empty
   bogus test - needs either switching to test_must_be_empty
   (and after that test_must_be_empty will report failure)
   or fixing generation of expect (and after that test result
   depends on contents).
 
 * both $1 and $2 are non-empty
   proceeding with test
   result depends on contents


Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

2018-08-19 Thread Andrei Rybak
On 17/08/18 22:09, Junio C Hamano wrote:
> Andrei Rybak  writes:
>>
>> I'll try something like the following on the weekend:
>>
>>  test_cmp () {
>>  if test "$1" != - && ! test -s "$1"
>>  then
>>  echo >&4 "error: trying to compare empty file '$1'"
>>  return 1
>>  fi
>>  if test "$2" != - && ! test -s "$2"
>>  then
>>  echo >&4 "error: trying to compare empty file '$2'"
>>  return 1
>>  fi
>>  test_cmp_allow_empty "$@"
>>  }
> 
> I actually think the above gives way too confusing output, when the
> actual output is empty and we are expecting some output.
> 
> The tester wants to hear from test_cmp "your 'git cmd' produced some
> output when we are expecting none" as the primary message.  We are
> trying to find bugs in "git" under development, and diagnosing iffy
> tests is secondary.  But with your change, the first thing that is
> checked is if 'expect' is an empty file and that is what we get
> complaints about, without even looking at what is in 'actual'.

I came up with two solutions for this issue:

  1. Check both files at the same time (combination with Gábor's
 function):

test_cmp () {
if test "$1" != - &&
   test "$2" != - &&
   ! test -s "$1" && 
   ! test -s "$2"
then
error "bug in test script: using test_cmp to check 
empty file; use test_must_be_empty instead"
fi
test_cmp_allow_empty "$@"
}

 This will still be reporting to the developer clearly, but
 will only catch cases exactly like the bogus test in t5310.

  2. Enable this check via variable, smth like EMPTY_CMP_LINT=1


Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

2018-08-17 Thread Andrei Rybak
On 17/08/18 19:39, SZEDER Gábor wrote:
> 
> See, we have quite a few tests that extract repetitive common tasks
> into helper functions, which sometimes includes preparing the expected
> results and running 'test_cmp', e.g. something like this
> (oversimplified) example:
> 
>   check_cmd () {
> git cmd $1 >actual &&
> echo "$2" >expect &&
> test_cmp expect actual
>   }
> 
>   check_cmd --fooFOO
>   check_cmd --no-foo ""

I've only had time to look into this from t0001 up to t0008-ignores.sh, where
test_check_ignore does this. If these helper functions need to allow comparing
empty files -- how about adding special variation of cmp functions for cases
like this: test_cmp_allow_empty and test_i18ncmp_allow_empty?

I think it would be a good trade-off to allow these helper functions to skip
checking emptiness of arguments for test_cmp. Such patch will require only
s/test_cmp/&_allow_empty/ for these helper functions and it will help catch
cases as bogus test in t5310.

I'll try something like the following on the weekend:

test_cmp() {
if test "$1" != - && ! test -s "$1"
then
echo >&4 "error: trying to compare empty file '$1'"
return 1
fi
if test "$2" != - && ! test -s "$2"
then
echo >&4 "error: trying to compare empty file '$2'"
return 1
fi
test_cmp_allow_empty "$@"
}

test_cmp_allow_empty() {
$GIT_TEST_CMP "$@"
}

(I'm not sure about redirections in test lib functions. The two if's would
probably be in a separate function to be re-used by test_i18ncmp.)


Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

2018-08-16 Thread Andrei Rybak
On 14/08/18 13:47, SZEDER Gábor wrote:
> ... both
> invocations produce empty 'pack{a,b}.objects' files, and the
> subsequent 'test_cmp' happily finds those two empty files identical.

Is test_cmp ever used for empty files? Would it make sense for
test_cmp to issue warning when an empty file is being compared?

> ---
>  t/t5310-pack-bitmaps.sh | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 6ee4d3f2d9..557bd0d0c0 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -9,7 +9,8 @@ objpath () {
>  
>  # show objects present in pack ($1 should be associated *.idx)
>  list_packed_objects () {
> - git show-index <"$1" | cut -d' ' -f2
> + git show-index <"$1" >object-list &&
> + cut -d' ' -f2 object-list
>  }
>  
>  # has_any pattern-file content-file
> @@ -204,8 +205,8 @@ test_expect_success 'pack-objects to file can use bitmap' 
> '
>   # verify equivalent packs are generated with/without using bitmap index
>   packasha1=$(git pack-objects --no-use-bitmap-index --all packa 
>packbsha1=$(git pack-objects --use-bitmap-index --all packb  &&
> - list_packed_objects packa.objects &&
> - list_packed_objects packb.objects &&
> + list_packed_objects packa-$packasha1.idx >packa.objects &&
> + list_packed_objects packb-$packbsha1.idx >packb.objects &&
>   test_cmp packa.objects packb.objects
>  '
>  
> 



[PATCH v2] line-log: clarify [a,b) notation for ranges

2018-08-08 Thread Andrei Rybak
line-log.[ch] use left-closed, right-open interval logic. Update
comments and debug output to use square brackets+parentheses notation
to help developers avoid off-by-one errors.

Signed-off-by: Andrei Rybak 
---

Applies on top of c0babbe69 (range-set: publish API for re-use by
git-blame -L, 2013-08-06). When applied on the original commit 12da1d1f6
(Implement line-history search (git log -L), 2013-03-28), the conflict
is in removal of "static" from some functions.

Changes since v1:

 - reword commit message a bit
 - sign-off
 - add [a,b) to comment of range_set_append_unsafe

I decided not to remove "_at the end_", as

  /* tack on a _new_ range [a,b) */

seems a bit less readable to me.

 line-log.c | 6 +++---
 line-log.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/line-log.c b/line-log.c
index fa9cfd5bd..9ab6d51cc 100644
--- a/line-log.c
+++ b/line-log.c
@@ -56,7 +56,7 @@ static void range_set_move(struct range_set *dst, struct 
range_set *src)
src->alloc = src->nr = 0;
 }
 
-/* tack on a _new_ range _at the end_ */
+/* tack on a _new_ range [a,b) _at the end_ */
 void range_set_append_unsafe(struct range_set *rs, long a, long b)
 {
assert(a <= b);
@@ -353,7 +353,7 @@ static void dump_range_set(struct range_set *rs, const char 
*desc)
int i;
printf("range set %s (%d items):\n", desc, rs->nr);
for (i = 0; i < rs->nr; i++)
-   printf("\t[%ld,%ld]\n", rs->ranges[i].start, rs->ranges[i].end);
+   printf("\t[%ld,%ld)\n", rs->ranges[i].start, rs->ranges[i].end);
 }
 
 static void dump_line_log_data(struct line_log_data *r)
@@ -373,7 +373,7 @@ static void dump_diff_ranges(struct diff_ranges *diff, 
const char *desc)
printf("diff ranges %s (%d items):\n", desc, diff->parent.nr);
printf("\tparent\ttarget\n");
for (i = 0; i < diff->parent.nr; i++) {
-   printf("\t[%ld,%ld]\t[%ld,%ld]\n",
+   printf("\t[%ld,%ld)\t[%ld,%ld)\n",
   diff->parent.ranges[i].start,
   diff->parent.ranges[i].end,
   diff->target.ranges[i].start,
diff --git a/line-log.h b/line-log.h
index e2a5ee7c6..488c86409 100644
--- a/line-log.h
+++ b/line-log.h
@@ -6,7 +6,7 @@
 struct rev_info;
 struct commit;
 
-/* A range [start,end].  Lines are numbered starting at 0, and the
+/* A range [start,end).  Lines are numbered starting at 0, and the
  * ranges include start but exclude end. */
 struct range {
long start, end;
-- 
2.18.0



[RFC PATCH] line-log: clarify [a,b) notation for ranges

2018-08-07 Thread Andrei Rybak
line-log.[ch] use left-closed, right-open interval logic. Change comment
and debug output to square brackets+parentheses notation to help
developers avoid off-by-one errors.
---

Original idea for this change in recent thread about line-log changes:

  https://public-inbox.org/git/9776862d-18b2-43ec-cfeb-829418d4d...@gmail.com/

line-log.c also uses ASCII graphics |---| in some comments, like:

/*
 * a: |---
 * b: --|
 */

But I'm not sure if replacing them with

/*
 * a: [---
 * b: --)
 */

will be helpful.  Another possibility is to update comment for
range_set_append_unsafe to read

  /* tack on a _new_ range [a,b) _at the end_ */
  void range_set_append_unsafe(struct range_set *rs, long a, long b)

 line-log.c | 4 ++--
 line-log.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/line-log.c b/line-log.c
index fa9cfd5bd..60f3174ac 100644
--- a/line-log.c
+++ b/line-log.c
@@ -353,7 +353,7 @@ static void dump_range_set(struct range_set *rs, const char 
*desc)
int i;
printf("range set %s (%d items):\n", desc, rs->nr);
for (i = 0; i < rs->nr; i++)
-   printf("\t[%ld,%ld]\n", rs->ranges[i].start, rs->ranges[i].end);
+   printf("\t[%ld,%ld)\n", rs->ranges[i].start, rs->ranges[i].end);
 }
 
 static void dump_line_log_data(struct line_log_data *r)
@@ -373,7 +373,7 @@ static void dump_diff_ranges(struct diff_ranges *diff, 
const char *desc)
printf("diff ranges %s (%d items):\n", desc, diff->parent.nr);
printf("\tparent\ttarget\n");
for (i = 0; i < diff->parent.nr; i++) {
-   printf("\t[%ld,%ld]\t[%ld,%ld]\n",
+   printf("\t[%ld,%ld)\t[%ld,%ld)\n",
   diff->parent.ranges[i].start,
   diff->parent.ranges[i].end,
   diff->target.ranges[i].start,
diff --git a/line-log.h b/line-log.h
index e2a5ee7c6..488c86409 100644
--- a/line-log.h
+++ b/line-log.h
@@ -6,7 +6,7 @@
 struct rev_info;
 struct commit;
 
-/* A range [start,end].  Lines are numbered starting at 0, and the
+/* A range [start,end).  Lines are numbered starting at 0, and the
  * ranges include start but exclude end. */
 struct range {
long start, end;
-- 
2.18.0



Re: [PATCH v3] t4150: fix broken test for am --scissors

2018-08-07 Thread Andrei Rybak
On 2018-08-06 22:14, Junio C Hamano wrote:
> Andrei Rybak  writes:
>>
>> Only changes since v2 are more clear tag names.
> 
> ... and updated log message, which I think makes it worthwhile to
> replace the previous one plus the squash/fixup with this version.

My bad, totally forgot that I had been tweaking it after I sent out v2.


[PATCH v3] t4150: fix broken test for am --scissors

2018-08-06 Thread Andrei Rybak
Tests for "git am --[no-]scissors" [1] work in the following way:

 1. Create files with commit messages
 2. Use these files to create expected commits
 3. Generate eml file with patch from expected commits
 4. Create commits using git am with these eml files
 5. Compare these commits with expected

The test for "git am --scissors" is supposed to take an e-mail with a
scissors line and in-body "Subject:" header and demonstrate that the
subject line from the e-mail itself is overridden by the in-body header
and that only text below the scissors line is included in the commit
message of the commit created by the invocation of "git am --scissors".
However, the setup of the test incorrectly uses a commit without the
scissors line and without the in-body header in the commit message,
producing eml file not suitable for testing of "git am --scissors".

This can be checked by intentionally breaking is_scissors_line function
in mailinfo.c, for example, by changing string ">8", which is used by
the test. With such change the test should fail, but does not.

Fix broken test by generating eml file with scissors line and in-body
header "Subject:". Since the two tests for --scissors and --no-scissors
options are there to test cutting or keeping the commit message, update
both tests to change the test file in the same way, which allows us to
generate only one eml file to be passed to git am. To clarify the
intention of the test, give files and tags more explicit names.

[1]: introduced in bf72ac17d (t4150: tests for am --[no-]scissors,
 2015-07-19)

Signed-off-by: Andrei Rybak 
---

Applies on top of 980a3d3dd (Merge branch 'pt/am-tests', 2015-08-03).
This patch is also available at

  https://github.com/rybak/git fix-am-scissors-test-v3

Only changes since v2 are more clear tag names.

 t/t4150-am.sh | 39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index e9b6f8158..a821dfda5 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -67,13 +67,15 @@ test_expect_success 'setup: messages' '
 
EOF
 
-   cat >scissors-msg <<-\EOF &&
-   Test git-am with scissors line
+   cat >msg-without-scissors-line <<-\EOF &&
+   Test that git-am --scissors cuts at the scissors line
 
This line should be included in the commit message.
EOF
 
-   cat - scissors-msg >no-scissors-msg <<-\EOF &&
+   printf "Subject: " >subject-prefix &&
+
+   cat - subject-prefix msg-without-scissors-line >msg-with-scissors-line 
<<-\EOF &&
This line should not be included in the commit message with --scissors 
enabled.
 
 - - >8 - - remove everything above this line - - >8 - -
@@ -150,18 +152,17 @@ test_expect_success setup '
} >patch1-hg.eml &&
 
 
-   echo scissors-file >scissors-file &&
-   git add scissors-file &&
-   git commit -F scissors-msg &&
-   git tag scissors &&
-   git format-patch --stdout scissors^ >scissors-patch.eml &&
+   echo file >file &&
+   git add file &&
+   git commit -F msg-without-scissors-line &&
+   git tag expected-for-scissors &&
git reset --hard HEAD^ &&
 
-   echo no-scissors-file >no-scissors-file &&
-   git add no-scissors-file &&
-   git commit -F no-scissors-msg &&
-   git tag no-scissors &&
-   git format-patch --stdout no-scissors^ >no-scissors-patch.eml &&
+   echo file >file &&
+   git add file &&
+   git commit -F msg-with-scissors-line &&
+   git tag expected-for-no-scissors &&
+   git format-patch --stdout expected-for-no-scissors^ 
>patch-with-scissors-line.eml &&
git reset --hard HEAD^ &&
 
sed -n -e "3,\$p" msg >file &&
@@ -418,10 +419,10 @@ test_expect_success 'am --scissors cuts the message at 
the scissors line' '
rm -fr .git/rebase-apply &&
git reset --hard &&
git checkout second &&
-   git am --scissors scissors-patch.eml &&
+   git am --scissors patch-with-scissors-line.eml &&
test_path_is_missing .git/rebase-apply &&
-   git diff --exit-code scissors &&
-   test_cmp_rev scissors HEAD
+   git diff --exit-code expected-for-scissors &&
+   test_cmp_rev expected-for-scissors HEAD
 '
 
 test_expect_success 'am --no-scissors overrides mailinfo.scissors' '
@@ -429,10 +430,10 @@ test_expect_success 'am --no-scissors overrides 
mailinfo.scissors' '
git reset --hard &&
git checkout second &&
test_config mailinfo.scissors true &&
-   git am --no-scissors no-scissors-patch.eml &&
+   git am --no-scissors patch-with-scissors-line.eml &&
test_path_is_missing .git/rebase-apply &&
-   git diff --exit-code no-scissors &&
-   test_cmp_rev no-scissors HEAD
+   git diff --exit-code expected-for-no-scissors &&
+   test_cmp_rev expected-for-no-scissors HEAD
 '
 
 test_expect_success 'setup: new author and committer' '
-- 
2.18.0


Re: [PATCH v2] t4150: fix broken test for am --scissors

2018-08-06 Thread Andrei Rybak
On 2018-08-06 10:58, Paul Tan wrote:
>> +   git commit -F msg-without-scissors-line &&
>> +   git tag scissors-used &&
> 
> Nit: I'm not quite sure about naming the tag "scissors-used" though,
> since this commit was not created from the output of "git am
> --scissors". Maybe it should be named `commit-without-scissors-line`
> or something?
> 
>> +   git commit -F msg-with-scissors-line &&
>> +   git tag scissors-not-used &&
> 
> Nit: Likewise, perhaps this tag could be named `commit-with-scissors-line`?

How about "expected-for-scissors" and "expected-for-no-scissors"?
Junio, I'll send out v3 with updated tag names, if that's OK.
Also, squash-able patch below.

> So, this patch fixes the 3 problems with the tests, and so looks correct to 
> me.

Thank you for such thorough review.

--- 8< ---
From: Andrei Rybak 
Date: Mon, 6 Aug 2018 19:29:03 +0200
Subject: [PATCH] squash! t4150: fix broken test for am --scissors

clarify tag names
---
 t/t4150-am.sh | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index bb2d951a70..a821dfda54 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -155,14 +155,14 @@ test_expect_success setup '
echo file >file &&
git add file &&
git commit -F msg-without-scissors-line &&
-   git tag scissors-used &&
+   git tag expected-for-scissors &&
git reset --hard HEAD^ &&
 
echo file >file &&
git add file &&
git commit -F msg-with-scissors-line &&
-   git tag scissors-not-used &&
-   git format-patch --stdout scissors-not-used^ 
>patch-with-scissors-line.eml &&
+   git tag expected-for-no-scissors &&
+   git format-patch --stdout expected-for-no-scissors^ 
>patch-with-scissors-line.eml &&
git reset --hard HEAD^ &&
 
sed -n -e "3,\$p" msg >file &&
@@ -421,8 +421,8 @@ test_expect_success 'am --scissors cuts the message at the 
scissors line' '
git checkout second &&
git am --scissors patch-with-scissors-line.eml &&
test_path_is_missing .git/rebase-apply &&
-   git diff --exit-code scissors-used &&
-   test_cmp_rev scissors-used HEAD
+   git diff --exit-code expected-for-scissors &&
+   test_cmp_rev expected-for-scissors HEAD
 '
 
 test_expect_success 'am --no-scissors overrides mailinfo.scissors' '
@@ -432,8 +432,8 @@ test_expect_success 'am --no-scissors overrides 
mailinfo.scissors' '
test_config mailinfo.scissors true &&
git am --no-scissors patch-with-scissors-line.eml &&
test_path_is_missing .git/rebase-apply &&
-   git diff --exit-code scissors-not-used &&
-   test_cmp_rev scissors-not-used HEAD
+   git diff --exit-code expected-for-no-scissors &&
+   test_cmp_rev expected-for-no-scissors HEAD
 '
 
 test_expect_success 'setup: new author and committer' '
-- 
2.18.0


Re: [PATCH 3/4] line-log: optimize ranges by joining them when possible

2018-08-05 Thread Andrei Rybak
On 2018-08-05 00:18, Johannes Schindelin via GitGitGadget wrote:
> 
> Now, I am fairly certain that the changes are correct, but given my track
> record with off-by-one bugs (and once even an off-by-two bug), I would
> really appreciate some thorough review of this code, in particular the
> second one that is the actual bug fix. I am specifically interested in
> reviews from people who know line-log.c pretty well and can tell me whether
> the src[i].end > target[j].end is correct, or whether it should actually
> have been a >= (I tried to wrap my head around this, but I would feel more
> comfortable if a domain expert would analyze this, whistling, and looking
> Eric's way).

I don't know line-log.c at all, but here are my comments on the more
abstract range and range_set changes:

On 2018-08-05 00:18, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin 
> 
> Technically, it is okay to have line ranges that touch (i.e. the end of
> the first range ends just before the next range begins). However, it is
> inefficient, and when the user provides such touching ranges via
> multiple `-L` options, we already join them.
>
> ...
>
>  void range_set_append(struct range_set *rs, long a, long b)
>  {
> + if (rs->nr > 0 && rs->ranges[rs->nr-1].end + 1 == a) {
> + rs->ranges[rs->nr-1].end = b;
> + return;
> + }

As I understand it, this patch attempts to make range_set_append extend
the last range in the range set to include [a,b), if [a,b) "touches" the
last range in rs.

Definition of range from line-log.h reads:

  /* A range [start,end].  Lines are numbered starting at 0, and the
   * ranges include start but exclude end. */
  struct range {
  long start, end;
  };

So the optimization described in commit message should take care of
following case, with zero lines between last range in rs and [a,b):

  rs before : [---) ... [---)
  [a,b) :   [---)
  rs after  : [---) ... [---)
  
It seems that the first condition in range_set_append should be:

if (rs->nr > 0 && rs->ranges[rs->nr-1].end == a) {
// extend the last range to include [a, b)
}

I think that the comments around struct range could be improved by
switching from using "[]", as in the comment from line-log.h quoted
above, and "|---|" as in various comments in line-log.c to "left-closed,
right-open" interval notation like "[start,end)" and "[---)".

>   assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a);
>   range_set_append_unsafe(rs, a, b);
>  }

With these consideration in mind the assert should become

assert(rs->nr == 0 || rs->ranges[rs->nr-1].end < a);
  
to cover cases starting from one line between last range in rs and [a,b)

  rs before : [---) ... [---)
  [a,b) :[---)
  rs after  : [---) ... [---)[---)
^
|
this line still not part of the range set.
  


[PATCH v2] t4150: fix broken test for am --scissors

2018-08-04 Thread Andrei Rybak
Tests for "git am --[no-]scissors" [1] work in the following way:

 1. Create files with commit messages
 2. Use these files to create expected commits
 3. Generate eml file with patch from expected commits
 4. Create commits using git am with these eml files
 5. Compare these commits with expected

The test for "git am --scissors" is supposed to take a message with a
scissors line and demonstrate that the subject line from the e-mail
itself is overridden by the in-body "Subject:" header and that only text
below the scissors line is included in the commit message of the commit
created by the invocation of "git am --scissors". However, the setup of
the test incorrectly uses a commit without the scissors line and in-body
"Subject:" header in the commit message, and thus, creates eml file not
suitable for testing of "git am --scissors".

This can be checked by intentionally breaking is_scissors_line function
in mailinfo.c, for example, by changing string ">8", which is used by
the test. With such change the test should fail, but does not.

Fix broken test by generating eml file with scissors line and in-body
header "Subject:". Since the two tests for --scissors and --no-scissors
options are there to test cutting or keeping the commit message, update
both tests to change the test file in the same way, which allows us to
generate only one eml file to be passed to git am. To clarify the
intention of the test, give files and tags more explicit names.

[1]: introduced in bf72ac17d (t4150: tests for am --[no-]scissors,
 2015-07-19)

Signed-off-by: Andrei Rybak 
---

Applies on top of 980a3d3dd (Merge branch 'pt/am-tests', 2015-08-03).
This patch is also available at

  https://github.com/rybak/git fix-am-scissors-test-v2

Changes since v1:

 - Reword commit message after feedback from Junio
 - Keep the empty line under scissors in the test e-mail, as it does not
   affect the test

 t/t4150-am.sh | 39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index e9b6f8158..bb2d951a7 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -67,13 +67,15 @@ test_expect_success 'setup: messages' '
 
EOF
 
-   cat >scissors-msg <<-\EOF &&
-   Test git-am with scissors line
+   cat >msg-without-scissors-line <<-\EOF &&
+   Test that git-am --scissors cuts at the scissors line
 
This line should be included in the commit message.
EOF
 
-   cat - scissors-msg >no-scissors-msg <<-\EOF &&
+   printf "Subject: " >subject-prefix &&
+
+   cat - subject-prefix msg-without-scissors-line >msg-with-scissors-line 
<<-\EOF &&
This line should not be included in the commit message with --scissors 
enabled.
 
 - - >8 - - remove everything above this line - - >8 - -
@@ -150,18 +152,17 @@ test_expect_success setup '
} >patch1-hg.eml &&
 
 
-   echo scissors-file >scissors-file &&
-   git add scissors-file &&
-   git commit -F scissors-msg &&
-   git tag scissors &&
-   git format-patch --stdout scissors^ >scissors-patch.eml &&
+   echo file >file &&
+   git add file &&
+   git commit -F msg-without-scissors-line &&
+   git tag scissors-used &&
git reset --hard HEAD^ &&
 
-   echo no-scissors-file >no-scissors-file &&
-   git add no-scissors-file &&
-   git commit -F no-scissors-msg &&
-   git tag no-scissors &&
-   git format-patch --stdout no-scissors^ >no-scissors-patch.eml &&
+   echo file >file &&
+   git add file &&
+   git commit -F msg-with-scissors-line &&
+   git tag scissors-not-used &&
+   git format-patch --stdout scissors-not-used^ 
>patch-with-scissors-line.eml &&
git reset --hard HEAD^ &&
 
sed -n -e "3,\$p" msg >file &&
@@ -418,10 +419,10 @@ test_expect_success 'am --scissors cuts the message at 
the scissors line' '
rm -fr .git/rebase-apply &&
git reset --hard &&
git checkout second &&
-   git am --scissors scissors-patch.eml &&
+   git am --scissors patch-with-scissors-line.eml &&
test_path_is_missing .git/rebase-apply &&
-   git diff --exit-code scissors &&
-   test_cmp_rev scissors HEAD
+   git diff --exit-code scissors-used &&
+   test_cmp_rev scissors-used HEAD
 '
 
 test_expect_success 'am --no-scissors overrides mailinfo.scissors' '
@@ -429,10 +430,10 @@ test_expect_success 'am --no-scissors overrides 
mailinfo.scissors' '
git reset --hard &a

Re: [PATCH] t4150: fix broken test for am --scissors

2018-08-03 Thread Andrei Rybak
On 2018-08-04 01:04, Junio C Hamano wrote:
> Hmph, I am not quite sure what is going on.  Is the only bug in the
> original that scissors-patch.eml and no-scissors-patch.eml files were
> incorrectly named?  IOW, if we fed no-scissors-patch.eml (which has
> a scissors line in it) with --scissors option to am, would it have
> worked just fine without other changes in this patch?

Just swapping eml files wouldn't be enough, because in old tests the
prepared commits touch different files: scissors-file and
no-scissors-file. And since tests are about cutting/keeping commit
message, it doesn't make much sense to keep two eml files which differ
only in contents of their diffs. I'll try to reword the commit message
to also include this bit.
 
> I am not saying that we shouldn't make other changes or renaming the
> confusing .eml files.  I am just trying to understand what the
> nature of the breakage was.  For example, it is not immediately
> obvious why the new test needs to prepare the message _with_
> "Subject:" in front of the first line when it prepares the commit
> to be used for testing.
> 
>   ... goes back and thinks a bit ...
> 
> OK, the Subject: thing that appears after the scissors line serves
> as an in-body header to override the subject line of the e-mail
> itself.  That change is necessary to _drop_ the subject from the
> outer e-mail and replace it with the subject we do want to use.
> 
> So I can explain why "Subject:" thing needed to be added.

Yes, the adding of "Subject: " prefix is completely overlooked in the
commit message. I'll add explanation in re-send.

> I cannot still explain why a blank line needs to be removed after
> the scissors line, though.  We should be able to have blank lines
> before the in-body header, IIRC.

I'll double check this and restore the blank line in v2, if the
removal is not needed. IIRC, I removed it by accident and didn't
think too much of it.

Thank you for review.


[PATCH] t4150: fix broken test for am --scissors

2018-08-03 Thread Andrei Rybak
Tests for "git am --[no-]scissors" [1] work in the following way:

 1. Create files with commit messages
 2. Use these files to create expected commits
 3. Generate eml file with patch from expected commits
 4. Create commits using git am with these eml files
 5. Compare these commits with expected

The test for "git am --scissors" is supposed to take a message with a
scissors line above commit message and demonstrate that only the text
below the scissors line is included in the commit created by invocation
of "git am --scissors".  However, the setup of the test uses commits
without the scissors line in the commit message, therefore creating an
eml file without scissors line.

This can be checked by intentionally breaking is_scissors_line function
in mailinfo.c. Test t4150-am.sh should fail, but does not.

Fix broken test by generating only one eml file--with scissors line, and
by using it both for --scissors and --no-scissors. To clarify the
intention of the test, give files and tags more explicit names.

[1]: introduced in bf72ac17d (t4150: tests for am --[no-]scissors,
     2015-07-19)

Signed-off-by: Andrei Rybak 
---

Applies on top of 980a3d3dd (Merge branch 'pt/am-tests', 2015-08-03).
This patch is also available at

  https://github.com/rybak/git fix-am-scissors-test


 t/t4150-am.sh | 40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index e9b6f8158..23e3b0e91 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -67,17 +67,18 @@ test_expect_success 'setup: messages' '
 
EOF
 
-   cat >scissors-msg <<-\EOF &&
-   Test git-am with scissors line
+   cat >msg-without-scissors-line <<-\EOF &&
+   Test that git-am --scissors cuts at the scissors line
 
This line should be included in the commit message.
EOF
 
-   cat - scissors-msg >no-scissors-msg <<-\EOF &&
+   printf "Subject: " >subject-prefix &&
+
+   cat - subject-prefix msg-without-scissors-line >msg-with-scissors-line 
<<-\EOF &&
This line should not be included in the commit message with --scissors 
enabled.
 
 - - >8 - - remove everything above this line - - >8 - -
-
EOF
 
signoff="Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
@@ -150,18 +151,17 @@ test_expect_success setup '
} >patch1-hg.eml &&
 
 
-   echo scissors-file >scissors-file &&
-   git add scissors-file &&
-   git commit -F scissors-msg &&
-   git tag scissors &&
-   git format-patch --stdout scissors^ >scissors-patch.eml &&
+   echo file >file &&
+   git add file &&
+   git commit -F msg-without-scissors-line &&
+   git tag scissors-used &&
git reset --hard HEAD^ &&
 
-   echo no-scissors-file >no-scissors-file &&
-   git add no-scissors-file &&
-   git commit -F no-scissors-msg &&
-   git tag no-scissors &&
-   git format-patch --stdout no-scissors^ >no-scissors-patch.eml &&
+   echo file >file &&
+   git add file &&
+   git commit -F msg-with-scissors-line &&
+   git tag scissors-not-used &&
+   git format-patch --stdout scissors-not-used^ 
>patch-with-scissors-line.eml &&
git reset --hard HEAD^ &&
 
sed -n -e "3,\$p" msg >file &&
@@ -418,10 +418,10 @@ test_expect_success 'am --scissors cuts the message at 
the scissors line' '
rm -fr .git/rebase-apply &&
git reset --hard &&
git checkout second &&
-   git am --scissors scissors-patch.eml &&
+   git am --scissors patch-with-scissors-line.eml &&
test_path_is_missing .git/rebase-apply &&
-   git diff --exit-code scissors &&
-   test_cmp_rev scissors HEAD
+   git diff --exit-code scissors-used &&
+   test_cmp_rev scissors-used HEAD
 '
 
 test_expect_success 'am --no-scissors overrides mailinfo.scissors' '
@@ -429,10 +429,10 @@ test_expect_success 'am --no-scissors overrides 
mailinfo.scissors' '
git reset --hard &&
git checkout second &&
test_config mailinfo.scissors true &&
-   git am --no-scissors no-scissors-patch.eml &&
+   git am --no-scissors patch-with-scissors-line.eml &&
test_path_is_missing .git/rebase-apply &&
-   git diff --exit-code no-scissors &&
-   test_cmp_rev no-scissors HEAD
+   git diff --exit-code scissors-not-used &&
+   test_cmp_rev scissors-not-used HEAD
 '
 
 test_expect_success 'setup: new author and committer' '
-- 
2.18.0




[RFC] broken test for git am --scissors

2018-08-03 Thread Andrei Rybak
I was tweaking is_scissors_line function in mailinfo.c and tried writing
some tests for it.  And discovered that existing test for git am option
--scissors is broken.  I then confirmed that by intentionally breaking
is_scissors_line, like this:

--- 8< ---
Subject: [PATCH 1/2] mailinfo.c: intentionally break is_scissors_line

It seems that tests for "git am" don't actually test the --scissor
option logic.  Break is_scissors_line function by using bogus symbols to
be able to check the tests.

Note that test suite does not pass with this patch applied.  The
expected failure does not happen.
---
 mailinfo.c| 4 ++--
 t/t4150-am.sh | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index 3281a37d5..7938d85e3 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -682,8 +682,8 @@ static int is_scissors_line(const char *line)
perforation++;
continue;
}
-   if ((!memcmp(c, ">8", 2) || !memcmp(c, "8<", 2) ||
-!memcmp(c, ">%", 2) || !memcmp(c, "%<", 2))) {
+   if ((!memcmp(c, ">o", 2) || !memcmp(c, "o<", 2) ||
+!memcmp(c, ">/", 2) || !memcmp(c, "/<", 2))) {
in_perforation = 1;
perforation += 2;
scissors += 2;
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 1ebc587f8..59bcb5afd 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -412,7 +412,8 @@ test_expect_success 'am with failing post-applypatch hook' '
test_cmp head.expected head.actual
 '
 
-test_expect_success 'am --scissors cuts the message at the scissors line' '
+# Test should fail, but succeeds
+test_expect_failure 'am --scissors cuts the message at the scissors line' '
rm -fr .git/rebase-apply &&
git reset --hard &&
git checkout second &&

--- >8 ---

Here's a proof-of-concept patch for the test, to make it actually fail
when is_scissors_line is broken.  It is the easiest way to do so, that I
could come up with, it is not ready to be applied.  I think the two
tests for --scissors should be rewritten pretty much from scratch, with
more obvious naming of files used.

(I made the changes to files in both tests the same just to be able to
re-use file "no-scissors-patch.eml", it's not relevant to the scissor
line in the commit messages.)

--- 8< ---
Subject: [PATCH 2/2] t4150-am.sh: fix test for --scissors

Test for option --scissors should check that the eml file with a scissor
line inside will be cut up and only the part under the cut will be
turned into commit.

However, the test for --scissors generates eml file without such line.
Fix the test for --scissors option.

Signed-off-by: Andrei Rybak 
---
 t/t4150-am.sh | 19 ---
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 59bcb5afd..5ad71fe05 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -69,17 +69,22 @@ test_expect_success 'setup: messages' '
 
EOF
 
+   cat >new-scissors-msg <<-\EOF &&
+   Subject: Test git-am with scissors line
+
+   This line should be included in the commit message.
+   EOF
+
cat >scissors-msg <<-\EOF &&
Test git-am with scissors line
 
This line should be included in the commit message.
EOF
 
-   cat - scissors-msg >no-scissors-msg <<-\EOF &&
+   cat - new-scissors-msg >no-scissors-msg <<-\EOF &&
This line should not be included in the commit message with --scissors 
enabled.
 
 - - >8 - - remove everything above this line - - >8 - -
-
EOF
 
signoff="Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
@@ -148,15 +153,15 @@ test_expect_success setup '
} >patch1-hg.eml &&
 
 
-   echo scissors-file >scissors-file &&
-   git add scissors-file &&
+   echo file >file &&
+   git add file &&
git commit -F scissors-msg &&
git tag scissors &&
git format-patch --stdout scissors^ >scissors-patch.eml &&
git reset --hard HEAD^ &&
 
-   echo no-scissors-file >no-scissors-file &&
-   git add no-scissors-file &&
+   echo file >file &&
+   git add file &&
git commit -F no-scissors-msg &&
git tag no-scissors &&
git format-patch --stdout no-scissors^ >no-scissors-patch.eml &&
@@ -417,7 +422,7 @@ test_expect_failure 'am --scissors cuts the message at the 
scissors line' '
rm -fr .git/rebase-apply &&
git reset --hard &&
git checkout second &&

Re: [PATCH 1/6] add, update-index: fix --chmod argument help

2018-08-02 Thread Andrei Rybak
On 2018-08-02 21:17, René Scharfe wrote:
> Don't translate the argument specification for --chmod; "+x" and "-x"
> are the literal strings that the commands accept.
> > [...]
> 
> - OPT_STRING( 0 , "chmod", _arg, N_("(+/-)x"), N_("override the 
> executable bit of the listed files")),
> + { OPTION_STRING, 0, "chmod", _arg, "(+|-)x",
> +   N_("override the executable bit of the listed files"),
> +   PARSE_OPT_LITERAL_ARGHELP },

Would it make sense to drop the localizations in po/* as well?
Or should such things be handled with l10n rounds?

Can be found using:

grep '(+/-)x' po/*


Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff

2018-07-26 Thread Andrei Rybak
On 2018-05-30 10:03, Eric Sunshine wrote:
> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named
> git-branch-diff[1] which computes differences between two versions of a
> patch series. Such a diff can be a useful aid for reviewers when
> inserted into a cover letter. However, doing so requires manual
> generation (invoking git-branch-diff) and copy/pasting the result into
> the cover letter.
> 
> This series fully automates the process by adding a --range-diff option
> to git-format-patch. 

[...]

> 
> Eric Sunshine (5):
>   format-patch: allow additional generated content in
> make_cover_letter()
>   format-patch: add --range-diff option to embed diff in cover letter
>   format-patch: extend --range-diff to accept revision range
>   format-patch: teach --range-diff to respect -v/--reroll-count
>   format-patch: add --creation-weight tweak for --range-diff
> 
>  Documentation/git-format-patch.txt |  18 +
>  builtin/log.c  | 119 -
>  t/t7910-branch-diff.sh |  16 
>  3 files changed, 132 insertions(+), 21 deletions(-)

Would it make sense to mention new option in the cover letter section of
Documentation/SubmittingPatches?

--
Best regards, Andrei Rybak


Re: [PATCH 9/9] diff.c: add white space mode to move detection that allows indent changes

2018-07-18 Thread Andrei Rybak
On 2018-07-17 01:05, Stefan Beller wrote:
> 
> This patch brings some challenges, related to the detection of blocks.
> We need a white net the catch the possible moved lines, but then need to

The s/white/wide/ was already suggested by Brandon Williams in previous
iteration, but it seems this also needs s/the catch/to catch/

> narrow down to check if the blocks are still in tact. Consider this
> example (ignoring block sizes):
> 


[PATCH] Documentation: fix --color option formatting

2018-07-18 Thread Andrei Rybak
Add missing colon in two places to fix formatting of options.

Signed-off-by: Andrei Rybak 
---

Done on top of maint.

The earliest this patch applies is on top of commit aebd23506e ("Merge
branch 'jk/ui-color-always-to-auto-maint' into
jk/ui-color-always-to-auto", 2017-10-04), one commit away from the
commit that added both of the affected lines: 0c88bf5050 (provide
--color option for all ref-filter users, 2017-10-03).

I grepped the Documentation folder, and haven't found any other
similar typos.

---

 Documentation/git-for-each-ref.txt | 2 +-
 Documentation/git-tag.txt  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 085d177d97..901faef1bf 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -57,7 +57,7 @@ OPTIONS
`xx`; for example `%00` interpolates to `\0` (NUL),
`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
 
---color[=]:
+--color[=]::
Respect any colors specified in the `--format` option. The
`` field must be one of `always`, `never`, or `auto` (if
`` is absent, behave as if `always` was given).
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 87c4288ffc..92f9c12b87 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -115,7 +115,7 @@ options for details.
variable if it exists, or lexicographic order otherwise. See
linkgit:git-config[1].
 
---color[=]:
+--color[=]::
Respect any colors specified in the `--format` option. The
`` field must be one of `always`, `never`, or `auto` (if
`` is absent, behave as if `always` was given).
-- 
2.18.0


Re: [RFC PATCH v2] Add 'human' date format

2018-07-11 Thread Andrei Rybak
On Wed, 11 Jul 2018 at 22:34, Andrei Rybak  wrote:
>
> Is -1 an OK initial value for timezone if local_time_tzoffset returns
> negative values as well? It looks like it doesn't matter for from functional
>

meant to say: "It looks like it doesn't matter from the functional
point of view".


Re: [RFC PATCH v2] Add 'human' date format

2018-07-11 Thread Andrei Rybak
On 2018-07-08 00:02, Linus Torvalds wrote:
> diff --git a/date.c b/date.c
> index 49f943e25..4486c028a 100644
> --- a/date.c
> +++ b/date.c
> @@ -77,22 +77,16 @@ static struct tm *time_to_tm_local(timestamp_t time)
>  }
>
>  /*
> - * What value of "tz" was in effect back then at "time" in the
> - * local timezone?
> + * Fill in the localtime 'struct tm' for the supplied time,
> + * and return the local tz.
>   */
> -static int local_tzoffset(timestamp_t time)
> +static int local_time_tzoffset(time_t t, struct tm *tm)
>  {
> - time_t t, t_local;
> - struct tm tm;
> + time_t t_local;
>   int offset, eastwest;
>
> - if (date_overflows(time))
> - die("Timestamp too large for this system: %"PRItime, time);
> -
> - t = (time_t)time;
> - localtime_r(, );
> - t_local = tm_to_time_t();
> -
> + localtime_r(, tm);
> + t_local = tm_to_time_t(tm);
>   if (t_local == -1)
>   return 0; /* error; just use + */
>   if (t_local < t) {
> @@ -107,6 +101,20 @@ static int local_tzoffset(timestamp_t time)
>   return offset * eastwest;
>  }
>

[...]

> +
>  const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
>  {
>   struct tm *tm;
> + struct tm human_tm = { 0 };
> + int human_tz = -1;

Is -1 an OK initial value for timezone if local_time_tzoffset returns
negative values as well? It looks like it doesn't matter for from functional

>   static struct strbuf timebuf = STRBUF_INIT;
>
>   if (mode->type == DATE_UNIX) {
> @@ -202,6 +281,15 @@ const char *show_date(timestamp_t time, int tz, const 
> struct date_mode *mode)
>   return timebuf.buf;
>   }
>
> + if (mode->type == DATE_HUMAN) {
> + struct timeval now;
> +
> + gettimeofday(, NULL);
> +
> + /* Fill in the data for "current time" in human_tz and human_tm 
> */
> + human_tz = local_time_tzoffset(now.tv_sec, _tm);
> + }
> +
>   if (mode->local)
>   tz = local_tzoffset(time);
>

--
Best regards, Andrei Rybak


[BUG] git cherry-pick does not complain about unknown options

2018-07-09 Thread Andrei Rybak
Hi,

I was trying to cherry pick commits, while simultaneously changing the
author.  Unfortunately, cherry-pick doesn't have the same --author
option as git-commit.  However, instead of complaining about unknown
option:

- when trying to cherry-pick one commit, it reported a BUG
- when trying to cherry-pick several commits, cherry-pick silently
  did nothing

All commits in tests existed in repository:

$ git cherry-pick --author='TEST'  # case 1
error: BUG: expected exactly one commit from walk
fatal: cherry-pick failed
$ echo $?
128
$ git cherry-pick --author='TEST'# case 2
$ echo $?
0
$ git --version
git version 2.18.0.windows.1

I've encountered this issue in Windows version, and  Johannes Schindelin
has confirmed that the issue is also present in Linux version.

Originally reported here: https://github.com/git-for-windows/git/issues/1751

--
Best regards, Andrei Rybak


Re: [PATCH v4 1/4] rebase: start implementing it as a builtin

2018-07-09 Thread Andrei Rybak
On 2018-07-08 20:01, Pratik Karki wrote:
> +
> +static int use_builtin_rebase(void)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf out = STRBUF_INIT;
> + int ret;
> +
> + argv_array_pushl(,
> +  "config", "--bool", "rebase.usebuiltin", NULL);
> + cp.git_cmd = 1;
> + if (capture_command(, , 6))
> + return 0;

Does strbuf out leak on return here?

> +
> + strbuf_trim();
> + ret = !strcmp("true", out.buf);
> + strbuf_release();
> + return ret;
> +}




Re: Incorrect unified diff when run with "--find-copies-harder"

2018-06-24 Thread Andrei Rybak
On 2018-06-24 12:36, Daniel Penkin wrote:
> Hello,
> 

Hi,

> I believe I found a bug in how Git represents a diff when invoked with
> "--find-copies-harder" parameter.
> Specifically, the unified diff header of a hunk contains an extra
> piece of text which appears to be a line from the context (i.e.
> unchanged line), something like this:
> 
> > git diff --find-copies-harder d00ca3f 20fb313
> diff --git a/test.txt b/copy.txt
> similarity index 81%
> copy from test.txt
> copy to copy.txt
> index 734156d..43a3f9d 100644
> --- a/test.txt
> +++ b/copy.txt
> @@ -2,6 +2,7 @@ line 1
>  line 2
>  line 3
>  line 4
> +added line
>  line 5
>  line 6
>  line 7
> 
> Note "line 1" after the standard unified diff header.
> 

This text after @@ is usually a function name in a programming language or
some other relevant part of hunk context, to help user navigate the diff more
easily.  What you are getting is the default version of it, as it is just
comparing txt files.  You can read more about it in the documentation of 
gitattributes:

https://git-scm.com/docs/gitattributes#_defining_a_custom_hunk_header

> I prepared a sample repository with a minimal file I can reproduce
> this problem with:
> https://bitbucket.org/dpenkin/find-copies-harder-bug
> 
> I'm running Git 2.18.0 on a macOS, but I also tried with Git 2.15.0
> and 2.8.6 running on Alpine Linux and was able to reproduce the same
> problem.
> 
> Please advise whether this is expected output or is indeed a bug.
> 

This is expected output.

> Thank you.
> 
> Kind regards,
> Daniil Penkin
> 

--
Best regards, Andrei R.


Re: Obsolete instruction in SubmittingPatches?

2018-03-01 Thread Andrei Rybak
On 01.03.2018 0:54, Junio C Hamano wrote:
> Andrei Rybak <rybak@gmail.com> writes:
> 
>> Is this part of guidelines obsolete, or am I not understanding this
>> correctly?
> 
> I am merely being nice (but only on "time-permitting" basis).
> 

Does that mean that the integration of a series is easier, when there is
a re-send?


Obsolete instruction in SubmittingPatches?

2018-02-28 Thread Andrei Rybak
Hello,

It seems to me that this part of Documentation/SubmittingPatches is
not actual nowadays:

  After the list reached a consensus that it is a good idea to apply the
  patch, re-send it with "To:" set to the maintainer{1} and "cc:" the
  list{2} for inclusion.

>From what I observe in the mailing list, most patches are sent with
"To:" set to mailing list (or re-sent with increasing version) , as per
previous paragraph in the guidelines [1].  Then, after the topic is
reviewed and the [PATCH v] series receives a thumbs up from a
number of people, the maintainer--Junio C Hamano--replies with an email
containing something along the lines of "This change is in good
shape.  Thanks, will queue.", which makes me think that the re-send
is not actually needed.

Is this part of guidelines obsolete, or am I not understanding this
correctly?

[1] "Send your patch with "To:" set to the mailing list, with "cc:"
listing people who are involved in the area you are touching"

--
Best regards, Andrei Rybak