Re: [PATCH] stash: fix show referencing stash index
On 6/15/19 1:26 PM, Thomas Gummerer wrote: > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index ea30d5f6a0..3973cbda0e 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -708,6 +708,24 @@ test_expect_success 'invalid ref of the form "n", n >= > N' ' > git stash drop > ' > > +test_expect_success 'valid ref of the form "n", n >= N' ' If ref is valid, 'n < N' was probably meant here.
Re: [PATCH] mailinfo: support Unicode scissors
On Mon, 1 Apr 2019 16:27:02 +0700, Duy Nguyen wrote: > On Mon, Apr 1, 2019 at 5:03 AM Andrei Rybak wrote: >> Support UTF-8 encoding of '✂' in function is_scissors_line, for 'git am >> --scissors' to be able to cut at Unicode perforation lines in emails. >> Note, that Unicode character '✂' is three bytes in UTF-8 encoding. > > On top of what was already said in this thread. For some reason (bad > font?) these scissors are drawn cutting _down_ for me instead of left > or right. It looks a bit strange. This might be an indication that the font used is rendering the symbol as an emoji. Most scissors in emoji fonts are vertical: https://emojipedia.org/black-scissors/
[PATCH v2 2/2] mailinfo: support Unicode scissors
Thank you all for review. Below is the second version of original patch,
addressing comments by Gábor and Peff.
While preparing v2 I found out that U+2702 was already suggested on the list
eight months before cutting at perforation lines was implemented:
https://public-inbox.org/git/200901181656.37813.markus.heidelb...@web.de/T/#m3856d2e5c5f3e1900210b74bf2be8851b92d2271
>8
Subject: [PATCH v2 2/2] mailinfo: support Unicode scissors
Date: Mon, 1 Apr 2019 00:00:00 +
'git am --scissors' allows cutting a patch from an email at a scissors
line. Such a line should contain perforation, i.e. hyphens, and a
scissors symbol. Only ASCII graphics scissors '8<' '>8' '%<' '>%' are
recognized by 'git am --scissors' command at the moment.
Unicode character 'BLACK SCISSORS' (U+2702) has been a part of Unicode
since version 1.0.0 [1]. Since then 'BLACK SCISSORS' also became part
of character set Emoji 1.0, published in 2015 [2]. With its adoption as
an emoji, availability of this character on keyboards has increased.
Support UTF-8 encoding of '✂' in function is_scissors_line, for 'git am
--scissors' to be able to cut at Unicode perforation lines in emails.
Note, that Unicode character '✂' is three bytes in UTF-8 encoding and is
spelled out using hexadecimal escape sequence.
1. https://www.unicode.org/versions/Unicode1.0.0/CodeCharts1.pdf
https://www.unicode.org/Public/reconstructed/1.0.0/UnicodeData.txt
2. https://unicode.org/Public/emoji/1.0/emoji-data.txt
Signed-off-by: Andrei Rybak
---
mailinfo.c| 7 +++
t/t4150-am.sh | 26 +-
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/mailinfo.c b/mailinfo.c
index f4aaa89788..804b07cd8a 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -701,6 +701,13 @@ static int is_scissors_line(const char *line)
c++;
continue;
}
+ if (starts_with(c, "\xE2\x9C\x82" /* U-2702 ✂ in UTF-8 */)) {
+ in_perforation = 1;
+ perforation += 3;
+ scissors += 3;
+ c += 2;
+ continue;
+ }
in_perforation = 0;
}
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 3f7f750cc8..3ea8e8a2cf 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -77,12 +77,20 @@ test_expect_success 'setup: messages' '
printf "Subject: " >subject-prefix &&
- cat - subject-prefix msg-without-scissors-line >msg-with-scissors-line
<<-\EOF
+ 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
+
+ cat - subject-prefix msg-without-scissors-line
>msg-with-unicode-scissors <<-\EOF
+ Lines above unicode scissors line should not be included in the commit
+ message with --scissors enabled.
+
+ - - - ✂ - - - ✂ - - -
+
+ EOF
'
test_expect_success setup '
@@ -161,6 +169,12 @@ test_expect_success setup '
git format-patch --stdout expected-for-no-scissors^
>patch-with-scissors-line.eml &&
git reset --hard HEAD^ &&
+ echo file >file &&
+ git add file &&
+ git commit -F msg-with-unicode-scissors &&
+ git format-patch --stdout HEAD^ >patch-with-unicode-scissors.eml &&
+ git reset --hard HEAD^ &&
+
sed -n -e "3,\$p" msg >file &&
git add file &&
test_tick &&
@@ -421,6 +435,16 @@ test_expect_success 'am --scissors cuts the message at the
scissors line' '
test_cmp_rev expected-for-scissors HEAD
'
+test_expect_success 'am --scissors cuts the message at the unicode scissors
line' '
+ rm -fr .git/rebase-apply &&
+ git reset --hard &&
+ git checkout second &&
+ git am --scissors patch-with-unicode-scissors.eml &&
+ test_path_is_missing .git/rebase-apply &&
+ git diff --exit-code expected-for-scissors &&
+ test_cmp_rev expected-for-scissors HEAD
+'
+
test_expect_success 'am --no-scissors overrides mailinfo.scissors' '
rm -fr .git/rebase-apply &&
git reset --hard &&
--
2.21.0
[PATCH v2 1/2] mailinfo: use starts_with() for clarity
Existing checks using memcmp(3) never read past the end of the line,
because all substrings we are interested in are two characters long, and
the outer loop guarantees we have at least one character. So at most we
will look at the NUL.
However, this is too subtle and may lead to bugs in code which copies
this behavior without realizing substring length requirement. So use
starts_with() instead, which will stop at NUL regardless of the length
of the prefix. Remove extra pair of parentheses while we are here.
Helped-by: Jeff King
Signed-off-by: Andrei Rybak
---
On Mon, Apr 01, 2019 at 06:11:57 -0400, Jeff King wrote:
> I wonder if it's worth re-writing it like:
Turned Peff's suggestion into a patch.
mailinfo.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mailinfo.c b/mailinfo.c
index b395adbdf2..f4aaa89788 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -693,8 +693,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 (starts_with(c, ">8") || starts_with(c, "8<") ||
+ starts_with(c, ">%") || starts_with(c, "%<")) {
in_perforation = 1;
perforation += 2;
scissors += 2;
--
2.21.0
[PATCH] mailinfo: support Unicode scissors
'git am --scissors' allows cutting a patch from an email at a scissors
line. Such a line should contain perforation, i.e. hyphens, and a
scissors symbol. Only ASCII graphics scissors '8<' '>8' '%<' '>%' are
recognized by 'git am --scissors' command at the moment.
Unicode character 'BLACK SCISSORS' (U+2702) has been a part of Unicode
since version 1.0.0 [1]. Since then 'BLACK SCISSORS' also became part
of character set Emoji 1.0, published in 2015 [2]. With its adoption as
an emoji, availability of this character on keyboards has increased.
Support UTF-8 encoding of '✂' in function is_scissors_line, for 'git am
--scissors' to be able to cut at Unicode perforation lines in emails.
Note, that Unicode character '✂' is three bytes in UTF-8 encoding.
1. https://www.unicode.org/versions/Unicode1.0.0/CodeCharts1.pdf
https://www.unicode.org/Public/reconstructed/1.0.0/UnicodeData.txt
2. https://unicode.org/Public/emoji/1.0/emoji-data.txt
Signed-off-by: Andrei Rybak
---
This applies on top of ar/t4150-remove-cruft merged into next in
commit a0106a8d5c, which also edited the test setup in t4150.
mailinfo.c| 7 +++
t/t4150-am.sh | 26 +-
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/mailinfo.c b/mailinfo.c
index b395adbdf2..4ef6cdee85 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -701,6 +701,13 @@ static int is_scissors_line(const char *line)
c++;
continue;
}
+ if (!memcmp(c, "✂", 3)) {
+ in_perforation = 1;
+ perforation += 3;
+ scissors += 3;
+ c++;
+ continue;
+ }
in_perforation = 0;
}
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 3f7f750cc8..3ea8e8a2cf 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -77,12 +77,20 @@ test_expect_success 'setup: messages' '
printf "Subject: " >subject-prefix &&
- cat - subject-prefix msg-without-scissors-line >msg-with-scissors-line
<<-\EOF
+ 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
+
+ cat - subject-prefix msg-without-scissors-line
>msg-with-unicode-scissors <<-\EOF
+ Lines above unicode scissors line should not be included in the commit
+ message with --scissors enabled.
+
+ - - - ✂ - - - ✂ - - -
+
+ EOF
'
test_expect_success setup '
@@ -161,6 +169,12 @@ test_expect_success setup '
git format-patch --stdout expected-for-no-scissors^
>patch-with-scissors-line.eml &&
git reset --hard HEAD^ &&
+ echo file >file &&
+ git add file &&
+ git commit -F msg-with-unicode-scissors &&
+ git format-patch --stdout HEAD^ >patch-with-unicode-scissors.eml &&
+ git reset --hard HEAD^ &&
+
sed -n -e "3,\$p" msg >file &&
git add file &&
test_tick &&
@@ -421,6 +435,16 @@ test_expect_success 'am --scissors cuts the message at the
scissors line' '
test_cmp_rev expected-for-scissors HEAD
'
+test_expect_success 'am --scissors cuts the message at the unicode scissors
line' '
+ rm -fr .git/rebase-apply &&
+ git reset --hard &&
+ git checkout second &&
+ git am --scissors patch-with-unicode-scissors.eml &&
+ test_path_is_missing .git/rebase-apply &&
+ git diff --exit-code expected-for-scissors &&
+ test_cmp_rev expected-for-scissors HEAD
+'
+
test_expect_success 'am --no-scissors overrides mailinfo.scissors' '
rm -fr .git/rebase-apply &&
git reset --hard &&
--
2.21.0
Re: [PATCH v5 26/26] doc: promote "git switch"
On 3/21/19 2:16 PM, Nguyễn Thái Ngọc Duy wrote:
> ...
>
> diff --git a/advice.c b/advice.c
> index b224825637..27e39e6514 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -191,20 +191,20 @@ void NORETURN die_conclude_merge(void)
> void detach_advice(const char *new_name)
> {
> const char *fmt =
> - _("Note: checking out '%s'.\n"
> + _("Note: switching to '%s'.\n"
> "\n"
> "You are in 'detached HEAD' state. You can look around, make
> experimental\n"
> "changes and commit them, and you can discard any commits you make in
> this\n"
> - "state without impacting any branches by performing another checkout.\n"
> + "state without impacting any branches by switching back to a branch.\n"
> "\n"
> "If you want to create a new branch to retain commits you create, you
> may\n"
> - "do so (now or later) by using -b with the checkout command again.
> Example:\n"
> + "do so (now or later) by using -c with the switch command. Example:\n"
> "\n"
> - " git checkout -b \n"
> + " git switch -c \n"
> "\n"
> "Or undo this checkout with:\n"
With the start of the message being "switching to ..." this part could probably
be also updated to something like "Or undo this switch with" or "Or undo this
switch or checkout with".
> "\n"
> - " git checkout -\n"
> + " git switch -\n"
> "\n"
> "Turn off this advice by setting config variable advice.detachedHead to
> false\n\n");
>
Re: [PATCH 2/2 v3] doc: format pathnames and URLs as monospace.
On 2019-03-12 16:48, Eric Sunshine wrote: > Thanks. A few comments: > > In patch 1/2: > > * drop the full stop from the first line of the commit message > > * s/futur/future/ in the commit message > > * s/There are false/& positives/ in the commit message > > * s/both, It/both, it/ Also, * s/inconsistant/inconsistent/ in the first paragraph of the commit message.
[PATCH] t4150: remove unused variable
In commit 735285b403 ("am: fix signoff when other trailers are present",
2017-08-08) tests using variable $signoff were rewritten and it is no
longer used, so just remove it from the test setup.
Signed-off-by: Andrei Rybak
---
t/t4150-am.sh | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 55b577d919..3f7f750cc8 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -77,14 +77,12 @@ test_expect_success 'setup: messages' '
printf "Subject: " >subject-prefix &&
- cat - subject-prefix msg-without-scissors-line >msg-with-scissors-line
<<-\EOF &&
+ 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>"
'
test_expect_success setup '
--
2.20.1
Re: [PATCH v3 19/21] t: add tests for switch
On 3/8/19 10:57 AM, Nguyễn Thái Ngọc Duy wrote: > --- > t/t2060-switch.sh | 87 +++ > 1 file changed, 87 insertions(+) > create mode 100755 t/t2060-switch.sh > > diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh > new file mode 100755 > index 00..1e1e834c1b > --- /dev/null > +++ b/t/t2060-switch.sh > @@ -0,0 +1,87 @@ > +#!/bin/sh > + [snip] > + > +test_expect_success 'switching ignores file of same branch name' ' > + test_when_finished git switch master && > + : >first-branch && > + git switch first-branch && > + echo refs/heads/first-branch >expected && > + git symbolic-ref HEAD >actual && > + test_commit expected actual s/commit/cmp/ > +' > + > +test_expect_success 'guess and create branch ' ' > + test_when_finished git switch master && > + test_must_fail git switch foo && > + git switch --guess foo && > + echo refs/heads/foo >expected && > + git symbolic-ref HEAD >actual && > + test_cmp expected actual > +' > + > +test_done >
Re: [PATCH v1 09/11] t: add tests for restore
On 3/8/19 11:16 AM, Nguyễn Thái Ngọc Duy wrote: > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > t/lib-patch-mode.sh | 12 > t/t2070-restore.sh (new +x) | 77 ++ > t/t2071-restore-patch.sh (new +x) | 105 ++ > 3 files changed, 194 insertions(+) > > diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh > new file mode 100755 > index 00..df91bf54bc > --- /dev/null > +++ b/t/t2070-restore.sh > @@ -0,0 +1,77 @@ > +#!/bin/sh > + > +test_description='restore basic functionality' > + > +. ./test-lib.sh > + > +test_expect_success 'setup' ' > + test_commit first && > + echo first-and-a-half >>first.t && > + git add first.t && > + test_commit second && > + echo one >one && > + echo two >two && > + echo untracked >untracked && > + echo ignored >ignored && > + echo /ignored >.gitignore && > + git add one two .gitignore && > + git update-ref refs/heads/one master > +' > + [snip] > + > +test_expect_success 'restore a file, ignoring branch of same name' ' > + cat one >expected && > + echo dirty >>one && > + git restore one && > + test_cmp expected one > +' > + Branch 'one' has been created by update-ref invocation in the setup, OK. > +test_expect_success 'restore a file on worktree from another branch' ' > + test_when_finished git reset --hard && > + git cat-file blob first:./first.t >expected && > + git restore --source=first first.t && > + test_cmp expected first.t && > + git cat-file blob HEAD:./first.t >expected && > + git show :first.t >actual && > + test_cmp expected actual > +' Test description reads "from another branch". However "first", created by test_commit invocation is a tag. Maybe description should read "from another ref"? Same applies to other tests which utilize "first". > + > +test_expect_success 'restore a file in the index from another branch' ' > + test_when_finished git reset --hard && > + git cat-file blob first:./first.t >expected && > + git restore --source=first --index first.t && > + git show :first.t >actual && > + test_cmp expected actual && > + git cat-file blob HEAD:./first.t >expected && > + test_cmp expected first.t > +' > + > +test_expect_success 'restore a file in both the index and worktree from > another branch' ' > + test_when_finished git reset --hard && > + git cat-file blob first:./first.t >expected && > + git restore --source=first --index --worktree first.t && > + git show :first.t >actual && > + test_cmp expected actual && > + test_cmp expected first.t > +' > + > +test_expect_success 'restore --index uses HEAD as source' ' > + test_when_finished git reset --hard && > + git cat-file blob :./first.t >expected && > + echo index-dirty >>first.t && > + git add first.t && > + git restore --index first.t && > + git cat-file blob :./first.t >actual && > + test_cmp expected actual > +' > + > +test_done -- Best regards, Andrei R.
Re: [PATCH] commit-tree: utilize parse-options api
A couple of code style issues:
On 2/26/19 9:09 PM, Brandon wrote:
> From: Brandon Richardson
>
> Rather than parse options manually, which is both difficult to
> read and error prone, parse options supplied to commit-tree
> using the parse-options api.
>
> It was discovered that the --no-gpg-sign option was documented
> but not implemented in 55ca3f99, and the existing implementation
> would attempt to translate the option as a tree oid.It was also
Missing space after period.
[snip]
> +
> int cmd_commit_tree(int argc, const char **argv, const char *prefix)
> {
> - int i, got_tree = 0;
> + static struct strbuf buffer = STRBUF_INIT;
> struct commit_list *parents = NULL;
> struct object_id tree_oid;
> struct object_id commit_oid;
> - struct strbuf buffer = STRBUF_INIT;
> +
> +struct option builtin_commit_tree_options[] = {
Style: tab should be used instead of four spaces.
> + { OPTION_CALLBACK, 'p', NULL, &parents, "parent",
> + N_("id of a parent commit object"), PARSE_OPT_NONEG,
Comparing to other similar places, a single tab should be used to
align "N_" instead of two spaces.
> + parse_parent_arg_callback },
> + { OPTION_CALLBACK, 'm', NULL, &buffer, N_("message"),
> + N_("commit message"), PARSE_OPT_NONEG,
> + parse_message_arg_callback },
> + { OPTION_CALLBACK, 'F', NULL, &buffer, N_("file"),
> + N_("read commit log message from file"), PARSE_OPT_NONEG,
> + parse_file_arg_callback },
> + { OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
> + N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) ""
> },
> + OPT_END()
> +};
[snip]
> -
> - if (!strcmp(arg, "--no-gpg-sign")) {
> - sign_commit = NULL;
> - continue;
> - }
> + argc = parse_options(argc, argv, prefix, builtin_commit_tree_options,
> + builtin_commit_tree_usage, 0);
here "builtin_commit_tree_usage" should be aligned with "argc" in
previous line.
Re: [PATCH v3 03/21] diff-parseopt: convert --dirstat and friends
On 2/16/19 12:36 PM, Nguyễn Thái Ngọc Duy wrote: > Signed-off-by: Nguyễn Thái Ngọc Duy > Signed-off-by: Junio C Hamano > --- > Documentation/diff-options.txt | 7 ++ > diff.c | 39 +- > 2 files changed, 36 insertions(+), 10 deletions(-) > > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt > index 0711734b12..058d93ec4f 100644 > --- a/Documentation/diff-options.txt > +++ b/Documentation/diff-options.txt > @@ -148,6 +148,7 @@ These parameters can also be set individually with > `--stat-width=`, > number of modified files, as well as number of added and deleted > lines. > > +-X:: > --dirstat[=]:: should probably marked as optional for -X > Output the distribution of relative amount of changes for each > sub-directory. The behavior of `--dirstat` can be customized by > @@ -192,6 +193,12 @@ directories with less than 10% of the total amount of > changed files, > and accumulating child directory counts in the parent directories: > `--dirstat=files,10,cumulative`. > > +--cumulative:: > + Synonym for --dirstat=cumulative > + > +--dirstat-by-file[=...]:: > + Synonym for --dirstat=files,param1,param2... > + > --summary:: > Output a condensed summary of extended header information > such as creations, renames and mode changes.
Re: [PATCH 29/59] config.txt: move i18n.* to a separate file
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
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!
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?
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?
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()
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
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
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
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
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
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
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
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
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
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
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
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-sc
Re: [PATCH] t4150: fix broken test for am --scissors
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
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
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 .g
Re: [PATCH 1/6] add, update-index: fix --chmod argument help
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", &chmod_arg, N_("(+/-)x"), N_("override the
> executable bit of the listed files")),
> + { OPTION_STRING, 0, "chmod", &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
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
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
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
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
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, &tm);
> - t_local = tm_to_time_t(&tm);
> -
> + localtime_r(&t, 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(&now, NULL);
> +
> + /* Fill in the data for "current time" in human_tz and human_tm
> */
> + human_tz = local_time_tzoffset(now.tv_sec, &human_tm);
> + }
> +
> if (mode->local)
> tz = local_tzoffset(time);
>
--
Best regards, Andrei Rybak
[BUG] git cherry-pick does not complain about unknown options
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
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(&cp.args,
> + "config", "--bool", "rebase.usebuiltin", NULL);
> + cp.git_cmd = 1;
> + if (capture_command(&cp, &out, 6))
> + return 0;
Does strbuf out leak on return here?
> +
> + strbuf_trim(&out);
> + ret = !strcmp("true", out.buf);
> + strbuf_release(&out);
> + return ret;
> +}
Re: Incorrect unified diff when run with "--find-copies-harder"
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?
On 01.03.2018 0:54, Junio C Hamano wrote: > Andrei Rybak 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?
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
