Re: [PATCH v1 02/11] restore: take tree-ish from --source option instead
On Fri, Mar 8, 2019 at 5:17 AM Nguyễn Thái Ngọc Duy wrote: > This is another departure from 'git checkout' syntax, which uses -- to > separate ref and pathspec. The observation is restore (or "git > checkout ,, ") is most often used to restore some files from What is the ",," thing? > the index. If this is correct, we can simplify it by taking a way the s/a way/away/ > ref, so that we can write > > git restore some-file > > without worrying about some-file being a ref and whether we need to do > > git restore -- some-file > > for safety. If the source of the restore comes from a tree, it will be > in the form of an option with value, e.g. > > git restore --source=this-tree some-file > > This is of course longer to type than using "--". But hopefully it > will not be used as often, and it is clearly easier to understand. > > dwim_new_local_branch is no longer set (or unset) in cmd_restore_files() > because it's irrelevant because we don't really care about dwim-ing. > With accept_ref being unset, dwim can't happen. > > Signed-off-by: Nguyễn Thái Ngọc Duy
[GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error
Hi, Thanks for the reviews. Here are the changes in the second version: - bug fixes - add preparatory patch - seperate different files to different patch - change to use test_line_count in a seperate patch Also I found that there is no such function as test_char_count, is it worthwile to add such function? Here are some stat: `git grep 'test_line_count' | wc -l` = 626 `git grep 'wc -l' | wc -l` = 294 `git grep 'wc -c' | wc -l` = 68 -- >8 -- This is a preparatory step prior to removing the pipes after git commands, which discards git's exit code and may mask a crash. Signed-off-by: Jonathan Chang diff --git a/t/t-basic.sh b/t/t-basic.sh index b6566003dd..53821f5817 100755 --- a/t/t-basic.sh +++ b/t/t-basic.sh @@ -1132,7 +1132,7 @@ test_expect_success 'git commit-tree records the correct parent in a commit' ' test_expect_success 'git commit-tree omits duplicated parent in a commit' ' commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) && -parent=$(git show --pretty=raw $commit2 | + parent=$(git show --pretty=raw $commit2 | sed -n -e "s/^parent //p" -e "/^author /q" | sort -u) && test "z$commit0" = "z$parent" && -- 2.21.0
[GSoC][PATCH v2 2/5] t0000-basic: avoid using pipes
The exit code of the upstream in a pipe is ignored thus we should avoid using it. By writing out the output of the git command to a file, we can test the exit codes of both the commands. Signed-off-by: Jonathan Chang diff --git a/t/t-basic.sh b/t/t-basic.sh index 53821f5817..47666b013e 100755 --- a/t/t-basic.sh +++ b/t/t-basic.sh @@ -1118,27 +1118,25 @@ P=$(test_oid root) test_expect_success 'git commit-tree records the correct tree in a commit' ' commit0=$(echo NO | git commit-tree $P) && - tree=$(git show --pretty=raw $commit0 | -sed -n -e "s/^tree //p" -e "/^author /q") && + git show --pretty=raw $commit0 >actual && + tree=$(sed -n -e "s/^tree //p" -e "/^author /q" actual) && test "z$tree" = "z$P" ' test_expect_success 'git commit-tree records the correct parent in a commit' ' commit1=$(echo NO | git commit-tree $P -p $commit0) && - parent=$(git show --pretty=raw $commit1 | - sed -n -e "s/^parent //p" -e "/^author /q") && + git show --pretty=raw $commit1 >actual && + parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual) && test "z$commit0" = "z$parent" ' test_expect_success 'git commit-tree omits duplicated parent in a commit' ' commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) && - parent=$(git show --pretty=raw $commit2 | - sed -n -e "s/^parent //p" -e "/^author /q" | - sort -u) && + git show --pretty=raw $commit2 >actual && + parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | sort -u) && test "z$commit0" = "z$parent" && - numparent=$(git show --pretty=raw $commit2 | - sed -n -e "s/^parent //p" -e "/^author /q" | - wc -l) && + git show --pretty=raw $commit2 >actual && + numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) && test $numparent = 1 ' @@ -1147,7 +1145,8 @@ test_expect_success 'update-index D/F conflict' ' mv path2 path0 && mv tmp path2 && git update-index --add --replace path2 path0/file2 && - numpath0=$(git ls-files path0 | wc -l) && + git ls-files path0 >actual && + numpath0=$(wc -l path4 && git update-index --add path4 && ( - git ls-files -s path4 | - sed -e "s/ .*/ /" | + git ls-files -s path4 >actual && + sed -e "s/ .*/ /" actual | tr -d "\012" && echo "$a" ) | git update-index --index-info && - len=$(git ls-files "a*" | wc -c) && + git ls-files "a*" >actual && + len=$(wc -c
[GSoC][PATCH v2 3/5] t0003-attributes: avoid using pipes
The exit code of the upstream in a pipe is ignored thus we should avoid using it. By writing out the output of the git command to a file, we can test the exit codes of both the commands. Signed-off-by: Jonathan Chang diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index 71e63d8b50..14274f1ced 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from stdin' ' test_expect_success 'attribute test: --all option' ' grep -v unspecified specified-all && sed -e "s/:.*//" stdin-all && - git check-attr --stdin --all actual && + git check-attr --stdin --all actual && + sort -o actual actual && test_cmp specified-all actual ' test_expect_success 'attribute test: --cached option' ' - git check-attr --cached --stdin --all actual && + git check-attr --cached --stdin --all actual && + sort -o actual actual && test_must_be_empty actual && git add .gitattributes a/.gitattributes a/b/.gitattributes && - git check-attr --cached --stdin --all actual && + git check-attr --cached --stdin --all actual && + sort -o actual actual && test_cmp specified-all actual ' @@ -301,8 +304,8 @@ test_expect_success 'bare repository: check that --cached honors index' ' ( cd bare.git && GIT_INDEX_FILE=../.git/index \ - git check-attr --cached --stdin --all <../stdin-all | - sort >actual && + git check-attr --cached --stdin --all <../stdin-all >actual && + sort -o actual actual && test_cmp ../specified-all actual ) ' -- 2.21.0
[GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes
The exit code of the upstream in a pipe is ignored thus we should avoid using it. By writing out the output of the git command to a file, we can test the exit codes of both the commands. Signed-off-by: Jonathan Chang diff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh index 7af3fbcc7b..05f443dcce 100755 --- a/t/t0022-crlf-rename.sh +++ b/t/t0022-crlf-rename.sh @@ -23,10 +23,10 @@ test_expect_success setup ' test_expect_success 'diff -M' ' - git diff-tree -M -r --name-status HEAD^ HEAD | - sed -e "s/R[0-9]*/RNUM/" >actual && + git diff-tree -M -r --name-status HEAD^ HEAD >actual && + sed -e "s/R[0-9]*/RNUM/" actual >output && echo "RNUM sample elpmas" >expect && - test_cmp expect actual + test_cmp expect output ' -- 2.21.0
[GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l
Signed-off-by: Jonathan Chang diff --git a/t/t-basic.sh b/t/t-basic.sh index 47666b013e..aa25694b45 100755 --- a/t/t-basic.sh +++ b/t/t-basic.sh @@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' ' parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | sort -u) && test "z$commit0" = "z$parent" && git show --pretty=raw $commit2 >actual && - numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) && - test $numparent = 1 + sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l >numparent && + test_line_count = 1 numparent ' test_expect_success 'update-index D/F conflict' ' @@ -1146,8 +1146,8 @@ test_expect_success 'update-index D/F conflict' ' mv tmp path2 && git update-index --add --replace path2 path0/file2 && git ls-files path0 >actual && - numpath0=$(wc -l numpath0 && + test_line_count = 1 numpath0 ' test_expect_success 'very long name in the index handled sanely' ' -- 2.21.0
Re: [GSoC][PATCH] tests: avoid using pipes
Thanks for the reminders.
On Sun, Mar 10, 2019 at 2:06 PM Christian Couder
wrote:
> If you take a look at c6f44e1da5 ("t9813: avoid using pipes",
> 2017-01-04) you can see the following:
>
> - it changes only one test file: t9813-git-p4-preserve-users.sh
> - its title starts with "t9813: "
I adapted this format in my second version[1].
[1]: https://public-inbox.org/git/20190310080739.63984-1-ttjt...@gmail.com
Thanks,
Jonathan
Re: [GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l
On Sun, Mar 10, 2019 at 4:11 AM Jonathan Chang wrote: > Signed-off-by: Jonathan Chang > > diff --git a/t/t-basic.sh b/t/t-basic.sh > @@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated > parent in a commit' ' > - numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc > -l) && > - test $numparent = 1 > + sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l >numparent > && > + test_line_count = 1 numparent This transformation makes no sense. The output of 'sed' is fed to 'wc -l' whose output is redirected to file "numparent", which means that the output file will end up containing a single line no matter how many "parent" lines are matched in the input. Since test_line_count() checks the number of lines in the named file, the test will succeed unconditionally (which makes for a pretty poor test). Also, the filename "numparent" doesn't do a good job of representing what the file is expected to contain. A better name might be "parents". So, a more correct transformation would be: sed -n -e "s/^parent //p" -e "/^author /q" actual >parents && test_line_count = 1 parents > @@ -1146,8 +1146,8 @@ test_expect_success 'update-index D/F conflict' ' > - numpath0=$(wc -l - test $numpath0 = 1 > + wc -l numpath0 && > + test_line_count = 1 numpath0 Same comment about bogus transformation. The entire sequence should collapse to a single line: test_line_count = 1 actual
Re: [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes
On Sun, Mar 10, 2019 at 4:10 AM Jonathan Chang wrote: > The exit code of the upstream in a pipe is ignored thus we should avoid > using it. By writing out the output of the git command to a file, we can > test the exit codes of both the commands. > > Signed-off-by: Jonathan Chang All of the patches in this series are malformed. There should be a "---" line right here below your sign-off. The "---" line is recognized by git-am/git-apply as separating the commit message from the actual diff(s). > diff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh > @@ -23,10 +23,10 @@ test_expect_success setup ' > test_expect_success 'diff -M' ' > > - git diff-tree -M -r --name-status HEAD^ HEAD | > - sed -e "s/R[0-9]*/RNUM/" >actual && > + git diff-tree -M -r --name-status HEAD^ HEAD >actual && > + sed -e "s/R[0-9]*/RNUM/" actual >output && > echo "RNUM sample elpmas" >expect && > - test_cmp expect actual > + test_cmp expect output It is a very well-established custom in Git tests for the files handed to test_cmp() to be named "expect" and "actual", so this change is not the most desirable. What you can do instead is: git diff-tree -M -r --name-status HEAD^ HEAD >output && sed -e "s/R[0-9]*/RNUM/" output >actual && which allows you to leave the test_cmp() line alone, thus (as a bonus) makes the patch less noisy.
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: [GSoC][PATCH v2 3/5] t0003-attributes: avoid using pipes
On Sun, Mar 10, 2019 at 4:09 AM Jonathan Chang wrote: > The exit code of the upstream in a pipe is ignored thus we should avoid > using it. By writing out the output of the git command to a file, we can > test the exit codes of both the commands. > > Signed-off-by: Jonathan Chang > > diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh > @@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from > stdin' ' > test_expect_success 'attribute test: --all option' ' > grep -v unspecified specified-all && > sed -e "s/:.*//" stdin-all && > - git check-attr --stdin --all actual && > + git check-attr --stdin --all actual && > + sort -o actual actual && > test_cmp specified-all actual > ' There is no existing use of "sort -o" anywhere in the Git test suite (or, for that matter, anywhere else in Git), which should give one pause before using it here. Although -o is allowed by POSIX, and POSIX even says it's safe for the output file to have the same name as one of the input files, there is no guarantee that "sort -o" will be supported on all platforms, or that all platforms promise that the output filename can match an input filename (in fact, neither the MacOS nor FreeBSD man pages for 'sort' make this promise). Consequently, it would be better to err on the side of safety and avoid "sort -o", which is easily enough done by using another temporary file: git check-attr --stdin --all output && sort output >actual && The same comment applies to the remaining changes.
Re: [PATCH v1 00/11] And new command "restore"
On Fri, Mar 8, 2019 at 5:17 PM Nguyễn Thái Ngọc Duy wrote: > - --index has a different meaning in git-apply. I don't have a good > suggestion except "yeah we have two different worlds now, the old > and the new one" I will rename --index to --staged to avoid this. It is already used as synonym for --cached in git-diff. I will also add --staged as synonym to "git rm" so that "git status" can consistently advise "git --staged" where verb is either restore or rm. Since option rename is a lot of work. If you think this is not a good move, raise it now, even if it's just "I don't like it, no reasons". -- Duy
[GSOC][PATCH] Fixed an issue which contained extra unnecessary code
>From 5a6c233c6bf0a35aca000b32b9e936a34532900a Mon Sep 17 00:00:00 2001
From: sushmaunnibhavi
Date: Sun, 10 Mar 2019 19:37:33 +0530
Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code
Signed-off-by: Sushma Unnibhavi
---
Since '\n' and '\0' are char_len==1,it is not necessary to check if the
char_len<=1.
compat/regex/regexec.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c
index 1b5d89fd5e..df62597531 100644
--- a/compat/regex/regexec.c
+++ b/compat/regex/regexec.c
@@ -3799,11 +3799,6 @@ check_node_accept_bytes (const re_dfa_t *dfa, int
node_idx,
char_len = re_string_char_size_at (input, str_idx);
if (node->type == OP_PERIOD)
{
- if (char_len <= 1)
- return 0;
- /* FIXME: I don't think this if is needed, as both '\n'
-and '\0' are char_len == 1. */
- /* '.' accepts any one character except the following two cases. */
if ((!(dfa->syntax & RE_DOT_NEWLINE) &&
re_string_byte_at (input, str_idx) == '\n') ||
((dfa->syntax & RE_DOT_NOT_NULL) &&
--
2.17.1
Re: [GSoC][PATCH] tests: avoid using pipes
On Sun, Mar 10, 2019 at 9:28 AM ttjtftx wrote:
>
> On Sun, Mar 10, 2019 at 2:06 PM Christian Couder
> wrote:
>
> > If you take a look at c6f44e1da5 ("t9813: avoid using pipes",
> > 2017-01-04) you can see the following:
> >
> > - it changes only one test file: t9813-git-p4-preserve-users.sh
> > - its title starts with "t9813: "
> I adapted this format in my second version[1].
>
> [1]: https://public-inbox.org/git/20190310080739.63984-1-ttjt...@gmail.com
It's better because each patch changes only one file, but I also
wanted to say that for a microproject you only need to focus on only
one file. So I would you suggest you keep only the patches that are
about "t-basic.sh" and drop the patches about
"t0003-attributes.sh" and "t0022-crlf-rename.sh".
(https://git.github.io/SoC-2019-Microprojects/ has now a "Only ONE
quality focused microproject per student" section with more content.)
It's also a small nit but your patches start with "t-basic: " not
just "t: " though the latter format is more frequent in existing
commits than the former.
[PATCH 0/2] fix spurious space after linkgit, now in *.html
On Sun, 3 Mar 2019 at 02:25, Junio C Hamano wrote: > > "brian m. carlson" writes: > > > On Wed, Feb 27, 2019 at 07:17:51PM +0100, Martin Ågren wrote: > >> Just like v1 [1], this v2 removes a spurious space which shows up in a > >> large number of places in our manpages when Asciidoctor expands the > >> linkgit:foo[bar] macro. [...] > > > > This version looks good to me. Thanks again for getting this cleaned up. > > Thanks, all. Will queue. Bleh. For some reason [1] I thought the html-files were exempt from this "extra space after linkgit" problem. They're not, as I just noticed. To add insult to injury, my original patch 2 which adds a missing dependency to the xml targets fails to add the exact same dependency for a few other targets. So of the three patches discussed above, at least two were incomplete. Since this has hit "next", here are two patches on top to address this. Sorry about this. Martin [1] I could have sworn I checked the html docs and saw that they didn't have this extra space. Looking at git-scm.com again reveals that it's not there. Huh. Seems like the site's html-rendering doesn't go through our Makefile at all and handles "linkgit:" on its own: https://github.com/git/git-scm.com/blob/master/script/doc_importer Martin Ågren (2): Documentation/Makefile: add missing dependencies on asciidoctor-extensions asciidoctor-extensions: fix spurious space after linkgit in *.html Documentation/Makefile | 6 +++--- Documentation/asciidoctor-extensions.rb | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) -- 2.21.0
[PATCH 2/2] asciidoctor-extensions: fix spurious space after linkgit in *.html
I really should have caught this when I wrote c3b4efa030
("asciidoctor-extensions: fix spurious space after linkgit",
2019-02-27). Turns out that when we create html-files, we take a
different path through this macro. So similar to c3b4efa030, we need to
drop a "\n" which turns into a space before punctuation.
Just like with c3b4efa030, note that if what follows is a word, not
punctuation, the white space renders just fine. So constructions such as
"see linkgit:foo[1] for more" still render as intended.
Signed-off-by: Martin Ågren
---
Documentation/asciidoctor-extensions.rb | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/asciidoctor-extensions.rb
b/Documentation/asciidoctor-extensions.rb
index f7a5982f8b..0089e0cfb8 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb
@@ -11,7 +11,7 @@ module Git
def process(parent, target, attrs)
if parent.document.basebackend? 'html'
prefix = parent.document.attr('git-relative-html-prefix')
- %(#{target}(#{attrs[1]})\n)
+ %(#{target}(#{attrs[1]}))
elsif parent.document.basebackend? 'docbook'
"\n" \
"#{target}" \
--
2.21.0
[PATCH 1/2] Documentation/Makefile: add missing dependencies on asciidoctor-extensions
I really should have caught this when I wrote 00c87bceaa
("Documentation/Makefile: add missing dependency on
asciidoctor-extensions", 2019-02-27). That commit made sure that the
xml-files depend on our Asciidoctor-specific extensions, but that just
helps for the ".txt -> .xml -> .[157]" transformations.
Because we produce the html-files directly as ".txt -> .html" -- not as
".txt -> .xml -> .html" --, we need to make the html-files too depend on
asciidoctor-extensions.rb.
There's one exception to the above paragraph. We do render
user-manual.html by going through an intermediate user-manual.xml, which
is an explicit, special-cased target. Add the missing dependency to
user-manual.xml.
Signed-off-by: Martin Ågren
---
Documentation/Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index f63c775e88..f58904a929 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -334,12 +334,12 @@ clean:
$(RM) $(cmds_txt) $(mergetools_txt) *.made
$(RM) manpage-base-url.xsl
-$(MAN_HTML): %.html : %.txt asciidoc.conf
+$(MAN_HTML): %.html : %.txt asciidoc.conf asciidoctor-extensions.rb
$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
$(TXT_TO_HTML) -d manpage -o $@+ $< && \
mv $@+ $@
-$(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
+$(OBSOLETE_HTML): %.html : %.txto asciidoc.conf asciidoctor-extensions.rb
$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
$(TXT_TO_HTML) -o $@+ $< && \
mv $@+ $@
@@ -356,7 +356,7 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
$(TXT_TO_XML) -d manpage -o $@+ $< && \
mv $@+ $@
-user-manual.xml: user-manual.txt user-manual.conf
+user-manual.xml: user-manual.txt user-manual.conf asciidoctor-extensions.rb
$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
$(TXT_TO_XML) -d book -o $@+ $< && \
mv $@+ $@
--
2.21.0
Re: New Ft. for Git : Allow resumable cloning of repositories.
On Fri, Mar 8, 2019 at 11:13 PM Jonathan Tan wrote: > This is indeed a nice feature to have, and thanks for details of how > this would be accomplished. > > One issue is that when cloning a repository, we do not download many > files - we only download one dynamically generated packfile containing > all the objects we want. Since the packfile is dynamically generated specifically for a client request, and is destroyed from the server as soon as the connection between them closes. Is this the reason why we cannot pause it in between like we can do with download managers ? I read through the progit ebook 'git internels' chapter and the following thought came to me: Assume a pack file as follows: --- $ git verify-pack -v .git/objects/pack/pack- 978e03944f5c581011e6998cd0e9e3905586.idx b042a60ef7dff760008df33cee372b945b6e884e blob 22054 5799 1463 033b4468fa6b2a9547a70d88d1bbe8bf3f9ed0d5 blob 9 20 7262 1 \ b042a60ef7dff760008df33cee372b945b6e884e .git/objects/pack/pack-978e03944f5c581011e6998cd0e9e3905586.pack: ok --- Here 033b blob refers b042 blob, and both blobs are different versions of the same file. Before this pack was made, both of these blobs were stored separately and thus were taking more space. Packfile is made to save space, by only storing latest version and its delta with earlier version. Both delta and latest version are stored in compressed form right ? Now, here is another approach to save space without needing to create pack: Earlier both the blobs had their object files as: .git/objects/03/3b4468fa6b2a9547a70d88d1bbe8bf3f9ed0d5 .git/objects/b0/42a60ef7dff760008df33cee372b945b6e884e Lets say b042 is latest and 033b is its earlier version. what git does in packfile can be done right here by: storing latest version in .git/objects/b0/42a60ef7dff760008df33cee372b945b6e884e and its delta in .git/objects/03/3b4468fa6b2a9547a70d88d1bbe8bf3f9ed0d5, with the delta version we can add a header that tells it to check for .git/objects/b0/42a60ef7dff760008df33cee372b945b6e884e and apply delta on it to get the earlier version. Doing this, eliminates the big packfile, and all the objects are spread into folders. We can now make this resume-able right ? Please point out what i missed here. Is it possible to do the above ? if yes then what was the reason to introduce concept of packfile ? > You might be interested in some work I'm doing to offload part of the > packfile response to CDNs: > > https://public-inbox.org/git/cover.1550963965.git.jonathanta...@google.com/ > > This means that when cloning/fetching, multiple files could be > downloaded, meaning that a scheme like you suggest would be more > worthwhile. (In fact, I allude to such a scheme in the design document > in patch 5.) currently reading through all the discussion on this strategy.
Re: [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error
On 03/10, Jonathan Chang wrote:
> Hi,
>
> Thanks for the reviews.
>
> Here are the changes in the second version:
> - bug fixes
> - add preparatory patch
> - seperate different files to different patch
> - change to use test_line_count in a seperate patch
>
> Also I found that there is no such function as test_char_count,
> is it worthwile to add such function? Here are some stat:
>
> `git grep 'test_line_count' | wc -l` = 626
> `git grep 'wc -l' | wc -l` = 294
> `git grep 'wc -c' | wc -l` = 68
I do think it would be helpful to introduce that helper, especially if
it is useful in this patch series. There seem to be enough other
places where it can be useful to make it worth adding the helper.
> -- >8 --
>
> This is a preparatory step prior to removing the pipes after git
> commands, which discards git's exit code and may mask a crash.
The commit message should also describe why we need this preparatory
step. Maybe something like:
To reduce the noise in when refactoring this pipeline in a
subsequent commit fix the indentation. This has been wrong
since the refactoring done in 1b5b2b641a ("t: modernise
style", 2012-03-02), but carries no meaning.
> Signed-off-by: Jonathan Chang
>
Out of curiosity, how did you create the patch? 'git format-patch'
would add a '---' line followed by the diffstat, where you would
usually put the commentary that you put before the scissors line. It
seems like 'git am' still handles this fine, which I didn't know, just
something I noticed because I'm used to the other format.
Since this patch series is now 5 patches, that commentary should go
into a cover letter (see the --cover-letter option in format-patch),
so the reviewers can read that first, and read the patches with that
in mind, focusing on the patch only, and not additional commentary
that applies to the whole series when reading the patch.
> diff --git a/t/t-basic.sh b/t/t-basic.sh
> index b6566003dd..53821f5817 100755
> --- a/t/t-basic.sh
> +++ b/t/t-basic.sh
> @@ -1132,7 +1132,7 @@ test_expect_success 'git commit-tree records the
> correct parent in a commit' '
>
> test_expect_success 'git commit-tree omits duplicated parent in a commit' '
> commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&
> - parent=$(git show --pretty=raw $commit2 |
> + parent=$(git show --pretty=raw $commit2 |
> sed -n -e "s/^parent //p" -e "/^author /q" |
> sort -u) &&
> test "z$commit0" = "z$parent" &&
> --
> 2.21.0
>
Re: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code
On Sun, Mar 10, 2019 at 4:30 PM sushmaunnibhavi
wrote:
>
> From 5a6c233c6bf0a35aca000b32b9e936a34532900a Mon Sep 17 00:00:00 2001
> From: sushmaunnibhavi
> Date: Sun, 10 Mar 2019 19:37:33 +0530
> Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code
> Signed-off-by: Sushma Unnibhavi
> ---
> Since '\n' and '\0' are char_len==1,it is not necessary to check if the
> char_len<=1.
> compat/regex/regexec.c | 5 -
> 1 file changed, 5 deletions(-)
It doesn't look like the patch is formatted correctly. I think that
the explanation line ("Since '\n' and '\0' are...") should be above
the line that contains your "Signed-off-by: ..." and there should be a
blank line between those two lines.
Also we ask for an author name in the "From: ..." header that looks
like "Firstname Lastname". A simple way to do that would be to make it
match the name in your "Signed-off-by: ...".
> diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c
> index 1b5d89fd5e..df62597531 100644
> --- a/compat/regex/regexec.c
> +++ b/compat/regex/regexec.c
> @@ -3799,11 +3799,6 @@ check_node_accept_bytes (const re_dfa_t *dfa, int
> node_idx,
>char_len = re_string_char_size_at (input, str_idx);
>if (node->type == OP_PERIOD)
> {
> - if (char_len <= 1)
> - return 0;
> - /* FIXME: I don't think this if is needed, as both '\n'
> -and '\0' are char_len == 1. */
> - /* '.' accepts any one character except the following two cases. */
>if ((!(dfa->syntax & RE_DOT_NEWLINE) &&
>re_string_byte_at (input, str_idx) == '\n') ||
> ((dfa->syntax & RE_DOT_NOT_NULL) &&
The code looks like:
char_len = re_string_char_size_at (input, str_idx);
if (node->type == OP_PERIOD)
{
if (char_len <= 1)
return 0;
/* FIXME: I don't think this if is needed, as both '\n'
and '\0' are char_len == 1. */
/* '.' accepts any one character except the following two cases. */
if ((!(dfa->syntax & RE_DOT_NEWLINE) &&
re_string_byte_at (input, str_idx) == '\n') ||
((dfa->syntax & RE_DOT_NOT_NULL) &&
re_string_byte_at (input, str_idx) == '\0'))
return 0;
return char_len;
}
If the byte at re_string_byte_at (input, str_idx) is indeed '\n' or
'\0', then yeah probably char_len == 1 so the current code should
return 0 just before the code below the FIXME comment is reached. So
in this case the 2 checks below the FIXME comment are useless because
they check re_string_byte_at (input, str_idx) == '\n') and
re_string_byte_at (input, str_idx) == '\0' which cannot happen.
So I would say that the right fix would be to remove those 2 checks,
not to remove the if (char_len <= 1) check above the FIXME comment.
Also note that I used "probably" when I wrote "then yeah probably
char_len == 1", because I think it is worth checking what
re_string_char_size_at() actually does before being too much
confident...
Re: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code
> Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code
Commit messages (and titles) should always be in the imperative mood.
The title in particular should be a short description of what the
patch is doing, and should give meaningful information to people
reading the output of 'git log --oneline'. Also we usually prefix the
title with the area of the code we're touching.
The title above is very generic, and thus not very useful to future
readers. Something like
compat/regex: remove unnecessary if
would give future readers a bit more context.
On 03/10, sushmaunnibhavi wrote:
> From 5a6c233c6bf0a35aca000b32b9e936a34532900a Mon Sep 17 00:00:00 2001
> From: sushmaunnibhavi
> Date: Sun, 10 Mar 2019 19:37:33 +0530
> Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code
The four lines above don't need to be included here, as they are
already defined by the email headers.
The one line that may be useful here is "From: sushmaunnibhavi
". That should be your full name, and
match the name in the Signed-off-by line. Usually people just set
that up in the "From" header in the email, but if you are unable to
configure your mailer like that, including a "From: Sushma Unnibhavi
" before the rest of the commit message
also works.
> Signed-off-by: Sushma Unnibhavi
> ---
> Since '\n' and '\0' are char_len==1,it is not necessary to check if the
> char_len<=1.
This explanation of why this is a good patch should be included in the
commit message before your Signed-off-by. If we want to apply this
change (which I don't think we want for the reasons stated below), I
think this needs a bit of a deeper analysis in the commit message, to
convince readers this is the correct thing to do.
Also please wrap commit messages at 72 characters.
> compat/regex/regexec.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c
This file in compat/ was directly imported from upstream gawk. We
generally don't patch this type of imported file, to make updates from
upstream easier, unless there is an actual fix from upstream that
needs to be fixed that's not going to be fixed upstream.
As this change at best removes a redundant 'if' (I can't comment on the
correctness, as I'm not familar with this code), so it is not worth
changing this file in our codebase.
> index 1b5d89fd5e..df62597531 100644
> --- a/compat/regex/regexec.c
> +++ b/compat/regex/regexec.c
> @@ -3799,11 +3799,6 @@ check_node_accept_bytes (const re_dfa_t *dfa, int
> node_idx,
>char_len = re_string_char_size_at (input, str_idx);
>if (node->type == OP_PERIOD)
> {
> - if (char_len <= 1)
> - return 0;
> - /* FIXME: I don't think this if is needed, as both '\n'
> - and '\0' are char_len == 1. */
> - /* '.' accepts any one character except the following two cases. */
>if ((!(dfa->syntax & RE_DOT_NEWLINE) &&
> re_string_byte_at (input, str_idx) == '\n') ||
> ((dfa->syntax & RE_DOT_NOT_NULL) &&
> --
> 2.17.1
>
Re: What's cooking in git.git (Mar 2019, #01; Wed, 6)
On 03/09, Elijah Newren wrote: > Hi, > > On Sat, Mar 9, 2019 at 9:29 AM Thomas Gummerer wrote: > > > > On 03/07, Duy Nguyen wrote: > > > On Thu, Mar 7, 2019 at 7:34 PM Philip Oakley wrote: > > > > The one point I noted is that "Overlay" is a new magic term without any > > > > explanation within the _documentation_. > > > > > > > > It would appear worth it to also add something (follow up patch?) to the > > > > e.g. git glossary to clarify how overlay differs, or is similar to, the > > > > different merge and reset modes. > > > > > > I think Jonathan questions the name "overlay" too. Since this is > > > similar to "cp -R" mode, another suggestion is --copy-mode. > > > > That would leave the negative form as --no-copy-mode, which almost > > sounds like we wouldn't copy anything. I think that would be more > > confusing that [no ]overlay mode. > > > > I'd be fine in general with changing the name, if there is a consensus > > for a different and better name, but I also think overlay is > > reasonable, so I'd rather leave pushing for a different name to > > someone else that has strong opinions about it. > > > > Philip, do you think something like this would help? Not sure if it > > should be called overlay_mode in the glossary, rather than just > > overlay? > > > > --- >8 --- > > Subject: [PATCH] glossary: add definition for overlay > > > > Add a definition for what overlay means in the context of git, to > > clarify the recently introduced overlay-mode in git checkout. > > > > Signed-off-by: Thomas Gummerer > > --- > > Documentation/glossary-content.txt | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/Documentation/glossary-content.txt > > b/Documentation/glossary-content.txt > > index 023ca95e7c..70e6477a9f 100644 > > --- a/Documentation/glossary-content.txt > > +++ b/Documentation/glossary-content.txt > > @@ -287,6 +287,11 @@ This commit is referred to as a "merge commit", or > > sometimes just a > > origin/name-of-upstream-branch, which you can see using > > `git branch -r`. > > > > +[[def_overlay]]overlay:: > > + Only update and add files to the working directory, but don't > > + delete them, similar to how 'cp -R' works. This is the > > + default in a <>. > > I think this is good, but maybe also add: >In contrast, no-overlay mode will also delete files not present in > the source, similar to 'rsync --delete' > ? Yeah, I think that makes sense, thanks! I'll wait a bit more to see if there are any other comments, and then send an updated version. > > + > > [[def_pack]]pack:: > > A set of objects which have been compressed into one file (to save > > space > > or to transmit them efficiently). > > -- > > 2.20.1.495.gaa96b0ce6b
Re: [PATCH v3 11/14] switch-branch: only allow explicit detached HEAD
On Thu, Nov 29, 2018 at 10:58:46PM +0100, Nguyễn Thái Ngọc Duy wrote:
> + if (!opts->implicit_detach &&
> + !opts->new_branch &&
> + !opts->new_branch_force &&
> + new_branch_info->name &&
> + !new_branch_info->path)
> + die(_("a branch is expected, got %s"), new_branch_info->name);
Wouldn't it be nice to give more context here, for example the symbolic
reference that the name actually points to? When expereimenting with the
feature and trying to switch to a tag, it refuses with
"a branch is expected, got v1.2.0". I personally would prefer something
more like "a branch is expected, got v1.2.0 that resolved to
refs/tags/v1.2.0", so I get "oh, yeah, that is actually a tag ...". Does
this seem worthwhile to dig deeper into? A quick glance left me a bit
puzzled, I admit.
Greetings,
Eckhard
Re: [PATCH v1 06/11] restore: add --worktree and --index
On Sat, Mar 9, 2019 at 1:52 PM Elijah Newren wrote:
> On Fri, Mar 8, 2019 at 2:17 AM Nguyễn Thái Ngọc Duy wrote:
> > + if (!opts->checkout_worktree && !opts->checkout_index)
> > + die(_("neither '%s' or '%s' is specified"),
> > + "--index", "--worktree");
>
> Is this die() or BUG()? I thought --worktree was the default.
The user might type "git restore --no-worktree --no-index" which would
trigger this error message, so definitely die(), not a BUG().
`git add <>` results in "fatal: ... is outside repository"
(In case 8.3 filename isn't a familiar term):
https://en.wikipedia.org/wiki/8.3_filename
This is distilled down from an actual issue to a minimal testcase.
The original failure involves an automated test while running on azure
pipelines: https://asottile.visualstudio.com/asottile/_build/results?buildId=254
I'm using git on windows
>git --version
git version 2.21.0.windows.1
Here's a minimal case:
git init longname-repo
cd longname-repo
touch f
git add ..\longna~1\f
Here's the output:
C:\Users\Anthony\AppData\Local\Temp\t\pre-commit-hooks>git init longname-repo
Initialized empty Git repository in
C:/Users/Anthony/AppData/Local/Temp/t/pre-commit-hooks/longname-repo/.git/
C:\Users\Anthony\AppData\Local\Temp\t\pre-commit-hooks>cd longname-repo
C:\Users\Anthony\AppData\Local\Temp\t\pre-commit-hooks\longname-repo>touch f
C:\Users\Anthony\AppData\Local\Temp\t\pre-commit-hooks\longname-repo>git
add ..\longna~1\f
fatal: ..\longna~1\f: '..\longna~1\f' is outside repository
It is however inside the repository:
C:\Users\Anthony\AppData\Local\Temp\t\pre-commit-hooks\longname-repo>python
Python 3.7.1 (v3.7.1:260ec2c36a, Oct 20 2018, 14:57:15) [MSC v.1915 64
bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.path.samefile('f', r'..\longna~1\f')
True
Anthony
Re: [PATCH v1 00/11] And new command "restore"
On March 10, 2019 4:19:28 AM PDT, Duy Nguyen wrote: >On Fri, Mar 8, 2019 at 5:17 PM Nguyễn Thái Ngọc Duy >wrote: >> - --index has a different meaning in git-apply. I don't have a good >> suggestion except "yeah we have two different worlds now, the old >> and the new one" > >I will rename --index to --staged to avoid this. It is already used as >synonym for --cached in git-diff. I will also add --staged as synonym >to "git rm" so that "git status" can consistently advise "git >--staged" where verb is either restore or rm. > >Since option rename is a lot of work. If you think this is not a good >move, raise it now, even if it's just "I don't like it, no reasons". For what it's worth, I think this is a good rename. Thanks, Jake -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH v3 10/21] checkout: split part of it to new command 'switch'
On March 8, 2019 1:57:41 AM PST, "Nguyễn Thái Ngọc Duy" wrote: >"git checkout" doing too many things is a source of confusion for many >users (and it even bites old timers sometimes). To remedy that, the >command will be split into two new ones: switch and >something-to-checkout-paths. The good old "git checkout" command is >still here and will be until all (or most of users) are sick of it. > >See the new man page for the final design of switch. The actual >implementation though is still pretty much the same as "git checkout" >and not completely aligned with the man page. Following patches will >adjust their behavior to match the man page. >--- > .gitignore| 1 + > Documentation/config/advice.txt | 13 +- > Documentation/config/branch.txt | 4 +- > Documentation/config/checkout.txt | 9 +- > Documentation/config/diff.txt | 3 +- > Documentation/git-checkout.txt| 4 + > Documentation/git-switch.txt | 259 ++ >+:: >+ Name for the new branch. >+ >+:: >+ The name of a commit at which to switch to before creating a >+ new branch or detach from. The wording here (and a few other places) feels awkward to me. I don't really have a better wording but maybe: --- The name of the commit to switch to when creating a new branch or detaching HEAD --- The original has weird tense when using detach. There were a few other places like this where the wording was "or detach from" but where the verb tense was confusing Thanks, Jake -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH v13 12/27] stash: convert drop and clear to builtin
On Sat, Mar 09, 2019 at 06:30:21PM +, Thomas Gummerer wrote: > On 03/07, Jeff King wrote: > > On Mon, Feb 25, 2019 at 11:16:16PM +, Thomas Gummerer wrote: > > > > > +static int do_drop_stash(const char *prefix, struct stash_info *info, > > > int quiet) > > > > This series hit next recently, so I started building it merged with my > > -Wunused-parameters series. This "prefix" parameter is not ever used. > > Skimming through the function, I don't see anything that _should_ be > > using it, so I think it's just cruft, and not indicative of a bug. > > Agreed, I think it's only cruft, and shouldn't be used anywhere in the > function. Below is a patch to remove the parameter. Thanks. I did something similar temporarily for my own tree, but I'll assume yours will go on top of the stash topic upstream. The patch below looks good (and matches what I did locally). > > The same is true of create_stash() elsewhere in the series. But there it > > might be worth keeping for consistency with the other top-level action > > functions. The other ones pass "prefix" to parse_options(), but > > create_stash() doesn't actually parse any options (and intentionally so, > > since even "--help" should be taken as part of the stash message). > > Agreed, I'd be happy to keep the parameter there. Looking at your > fork, you seem to have some WIP patches to introduce a UNUSED macro > for parameters like this, which I don't think I've seen on the list > yet (though I may have just missed them). > > I guess it's probably best for you to mark this parameter as UNUSED as > part of your series, but if you have a different preference on how to > handle it, let me know. Yep, that sounds good; I'll eventually mark this UNUSED when I send those patches. The "mark unused" ones haven't hit the list yet. I've been trickling the patches out, 10 or so at a time, but I'm still on the "drop ones that we can" patches, and haven't even gotten to the "mark ones we want to keep" patches. :) -Peff
Re: bitmaps by default? [was: prune: use bitmaps for reachability traversal]
On Sat, Mar 09, 2019 at 02:49:44AM +, Eric Wong wrote: > Jeff King wrote: > > Pruning generally has to traverse the whole commit graph in order to > > see which objects are reachable. This is the exact problem that > > reachability bitmaps were meant to solve, so let's use them (if they're > > available, of course). > > Perhaps this is good impetus for doing bitmaps by default? I'm actually not sure it is, because the prune costs less than making the bitmaps. Here are some timings on linux.git. This full-graph traversal is roughly the same cost as the reachability walk that prune would do internally: $ time git rev-list --objects --all >/dev/null real0m47.714s user0m47.113s sys 0m0.600s Here's a normal noop repack as a baseline. $ time git repack -ad real1m26.922s user1m20.029s sys 0m7.878s And here's another immediately after with bitmap generation. Generating the bitmaps takes about 100s, compared to the 47s it would save us on the prune. $ time git repack -adb real3m5.915s user2m59.377s sys 0m7.718s Things are a little rosier if you generate the bitmaps a second time: $ time git repack -adb real1m43.571s user1m37.403s sys 0m8.179s We can reuse some of the old bitmaps and it only takes 20 extra seconds, making it a net win. But I'm not sure how realistic that is. There were literally no new objects introduced between those two command. If this were a "real" repack occurring after we'd accumulated a week or two worth of objects, how long would it take? A few other random observations: - I do suspect there are some real inefficiencies in the way we generate bitmaps. It _should_ be about as expensive as the graph traversal, but clearly it's not. I think this is because of the way the current bitmap code picks commits to bitmap, and then walks somewhat haphazardly over the history, trying to accumulate the set of objects for each commit. IOW, I believe it may sometimes traverse over some sequences of history more than once. So if we could make that faster, then the balance would shift in its favor. - This is comparing the cost of generating the bitmaps to the time saved for _one_ operation. On a repo serving many fetches, the cost to generate it is amortized over many requests. But for a normal end-user, that's not true (they'd presumably push out their work, but that usually only needs to walk a small bit of history anyway). The balance would change if we had more operations that used bitmaps (e.g., --contains can use them, as can ahead/behind checks). We don't do those things yet, but we could. However, those algorithms are also using other commit-graph optimizations, and we've discussed revamping the bitmap format as part of that work (one problem in particular is that to use the current bitmaps you have to parse the whole .bitmap file, making it sometimes a net negative to use the bitmaps). So I'd consider holding off any decision like "let's make this the default" until we see where that work goes. > It would make life easier for people new to hosting git servers > (and hopefully reduce centralization :) I do think they're a net win for people hosting git servers. But if that's the goal, I think at most you'd want to make bitmaps the default for bare repos. They're really not much help for normal end-user repos at this point. > I started working on it, but t0410-partial-clone.sh fails with > "Failed to write bitmap index. Packfile doesn't have full > closure"; so more work needs to be done w.r.t. default behavior > on partial clones... Yeah, you can't use bitmaps at all in an incomplete clone. Shallow clones would probably have the same issue (though in general we just disable bitmaps entirely in shallow situations, so that might kick in). -Peff
Re: [PATCH v13 12/27] stash: convert drop and clear to builtin
Thomas Gummerer writes:
> Agreed, I'd be happy to keep the parameter there. Looking at your
> fork, you seem to have some WIP patches to introduce a UNUSED macro
> for parameters like this, which I don't think I've seen on the list
> yet (though I may have just missed them).
>
> I guess it's probably best for you to mark this parameter as UNUSED as
> part of your series, but if you have a different preference on how to
> handle it, let me know.
I agree that the uniformity among near-toplevel helpers like
create_stash() is a good thing to have.
In the meantime, you want the patch you sent (below) on top of the
stash-in-c topic to address do_drop_stash()?
Thanks for working well together.
> --- >8 ---
> Subject: [PATCH 2/2] stash: drop unused parameter
>
> Drop the unused prefix parameter in do_drop_stash.
>
> We also have an unused "prefix" parameter in the 'create_stash'
> function, however we leave that in place for symmetry with the other
> top-level functions.
>
> Reported-by: Jeff King
> Signed-off-by: Thomas Gummerer
> ---
> builtin/stash.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 6eb67c75c3..069bf14846 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -527,7 +527,7 @@ static int apply_stash(int argc, const char **argv, const
> char *prefix)
> return ret;
> }
>
> -static int do_drop_stash(const char *prefix, struct stash_info *info, int
> quiet)
> +static int do_drop_stash(struct stash_info *info, int quiet)
> {
> int ret;
> struct child_process cp_reflog = CHILD_PROCESS_INIT;
> @@ -597,7 +597,7 @@ static int drop_stash(int argc, const char **argv, const
> char *prefix)
>
> assert_stash_ref(&info);
>
> - ret = do_drop_stash(prefix, &info, quiet);
> + ret = do_drop_stash(&info, quiet);
> free_stash_info(&info);
> return ret;
> }
> @@ -626,7 +626,7 @@ static int pop_stash(int argc, const char **argv, const
> char *prefix)
> printf_ln(_("The stash entry is kept in case "
> "you need it again."));
> else
> - ret = do_drop_stash(prefix, &info, quiet);
> + ret = do_drop_stash(&info, quiet);
>
> free_stash_info(&info);
> return ret;
> @@ -663,7 +663,7 @@ static int branch_stash(int argc, const char **argv,
> const char *prefix)
> if (!ret)
> ret = do_apply_stash(prefix, &info, 1, 0);
> if (!ret && info.is_stash_ref)
> - ret = do_drop_stash(prefix, &info, 0);
> + ret = do_drop_stash(&info, 0);
>
> free_stash_info(&info);
Re: [PATCH v13 18/27] stash: convert create to builtin
Thomas Gummerer writes: > Subject: [PATCH 1/2] stash: pass pathspec as pointer > > Passing the pathspec by value is potentially confusing, as the copy is > only a shallow copy, so save the overhead of the copy, and pass the > pathspec struct as a pointer. > > In addition use copy_pathspec to copy the pathspec into > rev.prune_data, so the copy is a proper deep copy, and owned by the > revision API, as that's what the API expects. It does make quite a lot of sense, but do we need clear_pathspec() at strategic places after we are done using these copied instances?
Re: [PATCH v2] line-log: suppress diff output with "-s"
Jeff King writes:
> On Fri, Mar 08, 2019 at 04:38:44PM +0100, Johannes Schindelin wrote:
>
>> On Thu, 7 Mar 2019, Jeff King wrote:
>>
>> > When "-L" is in use, we ignore any diff output format that the user
>> > provides to us, and just always print a patch (with extra context lines
>> > covering the whole area of interest). It's not entirely clear what we
>> > should do with all formats (e.g., should "--stat" show just the diffstat
>> > of the touched lines, or the stat for the whole file?).
>> >
>> > But "-s" is pretty clear: the user probably wants to see just the
>> > commits that touched those lines, without any diff at all. Let's at
>> > least make that work.
>>
>> Agree. The patch looks obviously good.
>
> Thanks. This leaves the other formats as silently ignored. Do we want to
> do something like this:
It probably would make sense to do this if only to avoid surprises.
>
> diff --git a/revision.c b/revision.c
> index eb8e51bc63..a1b4fe2aa6 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2689,6 +2689,10 @@ int setup_revisions(int argc, const char **argv,
> struct rev_info *revs, struct s
> if (revs->first_parent_only && revs->bisect)
> die(_("--first-parent is incompatible with --bisect"));
>
> + if (revs->line_level_traverse &&
> + (revs->diffopt.output_format &
> ~(DIFF_FORMAT_PATCH|DIFF_FORMAT_NO_OUTPUT)))
> + die(_("-L does not yet support diff formats besides -p and
> -s"));
> +
> if (revs->expand_tabs_in_log < 0)
> revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
>
>
> ?
>
> -Peff
Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s
Eric Sunshine writes:
> On Fri, Mar 8, 2019 at 12:38 AM Junio C Hamano wrote:
>> An unrelated tangent, but what do you think of this patch? In the
>> context of testing "git rm", if foo is a dangling symbolic link,
>> "git rm foo && test_path_is_missing foo" would need something like
>> this to work correctly, I would think.
>>
>> test_path_is_missing () {
>> - if test -e "$1"
>> + if test -e "$1" || test -L "$1"
>> then
>> echo "Path exists:"
>> ls -ld "$1"
>
> Makes sense. Won't we also want:
>
> test_path_exists () {
> -if ! test -e "$1"
> + if ! test -e "$1" && ! test -L "$1"
>then
> echo "Path $1 doesn't exist. $2"
>
> or something like that?
That would make them symmetric, but what I was driving at with "In
the context of testing git rm" was that I highly suspect that among
other existing users of test_path_is_missing there are some that
want to consider a dangling symbolic link as if it is not there (and
vice versa for test_path_exists), and it may not be a good idea to
unconditionally declare that, unlike the underlying "test" command
that dereferences symlinks for most operations, our wrapper does not
dereference symbolic links, which is what the "what do you think?"
patch and your addtion do.
Re: [PATCH v3 1/2] worktree: fix worktree add race.
Duy Nguyen writes: > Junio, it seems 2/2 is stuck in an endless discussion. But 1/2 is good > regardless, maybe pick it up now and let 2/2 come later whenever it's > ready? Thanks for poking, and I think it is a good idea.
Re: One failed self test on Fedora 29
Jeffrey Walton writes: > I think this is the patch for sha1dc/sha1.c . It stops using unaligned > accesses by default, but still honors SHA1DC_FORCE_UNALIGNED_ACCESS > for those who want it. Folks who want the undefined behavior have to > do something special. Hmph, I somehow thought that folks who want to stick to the standard printed on paper penalizing what practicaly works well in the real world would be the one doing extra things.
Re: One failed self test on Fedora 29
On Sun, Mar 10, 2019 at 10:00 PM Junio C Hamano wrote: > > Jeffrey Walton writes: > > > I think this is the patch for sha1dc/sha1.c . It stops using unaligned > > accesses by default, but still honors SHA1DC_FORCE_UNALIGNED_ACCESS > > for those who want it. Folks who want the undefined behavior have to > > do something special. > > Hmph, I somehow thought that folks who want to stick to the > standard printed on paper penalizing what practicaly works well in > the real world would be the one doing extra things. In the paper example, we do. I prefer printed books. Just last month I went to Office Depot and had portions of an ARM manual distributed as PDF printed and spiral bound. I don't mind doing extra work as the special case. Jeff
Re: is it "git gc --prune=now" or "git gc --prune=all"?
"Robert P. J. Day" writes: > On Wed, 6 Mar 2019, Junio C Hamano wrote: > >> Jeff King writes: >> >> > ... I do think the documentation should recommend "now". Possibly >> > builtin/gc.c should be smarter about recognizing "all" in the >> > conditional you quoted, too, though I don't know that it's all >> > that important (especially if we tweak the documentation). >> >> Yup, as the placeholder for the value is labeled as "", "now" >> would be the one we should be encouraging. > > i can submit an obviously trivial patch for that You already did to turn --prune=all into --prune=now in the doc, I think. Do we encourage 'false' where we should encourage 'never'? I see we do not even mention the negative form in git-gc manpage, and I think it is a good thing, especially because --prune= is listed just next to --no-prune, which is even more explicit.
Re: [PATCH/RFC v1 1/1] convert.c: Escape sequences only for a tty in trace_encoding()
tbo...@web.de writes:
> I am temped to remove the "dim" functionality all together,
> or to remove the printout of the values which are now dimmed,
> what do others think ?
I am for removing the color settings we see here for two reasons.
One is that the tracing is primarily for machine readability.
Another is that if we want to have an option to make the output more
human friendly, we should have (a) a facility for users of the
tracing mechanism, like the codepath we see here, to mark the parts
of its output in a more logical ways (e.g. "here comes a less
important piece of info"), (b) a knob to tell the tracing mechanism
what kind of output (e.g. "more friendly to humans using color") is
requested, and (c) code in the tracing mechanism (e.g. the helper
functions implemented at the same level as trace_printf()), and what
we have are far from such a structured thing---and then such a new
effort for more structured tracing should probably go to the trace2
effort jeffhost is spearheading.
But I am speaking as just one of the reviewers here, and if people
feel differently, I would want to hear it first before deciding
anything.
Thanks.
> convert.c | 20
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index 5d0307fc10..70e58f1413 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -42,6 +42,9 @@ struct text_stat {
> unsigned printable, nonprintable;
> };
>
> +static const char *terminal_half_bright;
> +static const char *terminal_reset_normal;
> +
> static void gather_stats(const char *buf, unsigned long size, struct
> text_stat *stats)
> {
> unsigned long i;
> @@ -330,14 +333,23 @@ static void trace_encoding(const char *context, const
> char *path,
> static struct trace_key coe = TRACE_KEY_INIT(WORKING_TREE_ENCODING);
> struct strbuf trace = STRBUF_INIT;
> int i;
> -
> + if (!terminal_half_bright || !terminal_reset_normal) {
> + if (isatty(1)) {
> + terminal_half_bright = "\033[2m";
> + terminal_reset_normal = "\033[0m";
> + } else {
> + terminal_half_bright = "";
> + terminal_reset_normal = "";
> + }
> + }
> strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path,
> encoding);
> for (i = 0; i < len && buf; ++i) {
> + char c = buf[i] > 32 && buf[i] < 127 ? buf[i] : ' ';
> strbuf_addf(
> - &trace, "| \033[2m%2i:\033[0m %2x \033[2m%c\033[0m%c",
> - i,
> + &trace, "| %s%2i:%s %2x %s%c%s%c",
> + terminal_half_bright, i, terminal_reset_normal,
> (unsigned char) buf[i],
> - (buf[i] > 32 && buf[i] < 127 ? buf[i] : ' '),
> + terminal_half_bright, c, terminal_reset_normal,
> ((i+1) % 8 && (i+1) < len ? ' ' : '\n')
> );
> }
> --
> 2.21.0.135.g6e0cc67761
Re: [PATCH 0/2] fix spurious space after linkgit, now in *.html
On Sun, Mar 10, 2019 at 04:14:22PM +0100, Martin Ågren wrote: > [1] I could have sworn I checked the html docs and saw that they didn't > have this extra space. Looking at git-scm.com again reveals that it's > not there. Huh. Seems like the site's html-rendering doesn't go through our > Makefile at all and handles "linkgit:" on its own: > https://github.com/git/git-scm.com/blob/master/script/doc_importer Correct. It significantly predates any support for asciidoctor in our Makefile, and needs to do some custom ruby-level tweaking (though possibly that could be cleaned up these days). The doc_importer script you found, though, is not part of that process. I _think_ it's just leftover cruft. The actual import and conversion happens as part of the rake task in: https://github.com/git/git-scm.com/blob/master/lib/tasks/index.rake (Not that any of this matters for your series; just sprinkling some fun facts into the conversation). -Peff
Re: [PATCH 0/2] fix spurious space after linkgit, now in *.html
Martin Ågren writes: > Bleh. For some reason [1] I thought the html-files were exempt from this > "extra space after linkgit" problem. They're not, as I just noticed. To > add insult to injury, my original patch 2 which adds a missing > dependency to the xml targets fails to add the exact same dependency for > a few other targets. So of the three patches discussed above, at least > two were incomplete. > > Since this has hit "next", here are two patches on top to address this. > > Sorry about this. Thanks. If it makes it easier, we can eject ma/asciidoctor-fixes out of 'next' when we rebuild 'next'. I actually am tempted to start 'next' from empty for this round after merging some obviously correct ones that are there to the 'master' branch.
Re: One failed self test on Fedora 29
On Sat, Mar 09, 2019 at 07:34:15AM -0500, Jeffrey Walton wrote: > > It would probably help to know what commit you're building. > > The verbose test output would also be useful, e.g.: > > I built with CFLAGS += -fsanitize=undefined. It looks like the > misaligned accesses generate UBsan findings, which is causing > t0021-conversion to fail. You probably should use SANITIZE=undefined instead. The Makefile has some smarts to tweak build parameters based on your sanitize flag (e.g., defining NO_UNALIGNED_LOADS). That said, I do not think sha1dc works with UBsan at this point at all. I usually do error-checking builds with: make SANITIZE=address,undefined BLK_SHA1=Yes What puzzles me is not that t0021 failed, but that everything else _didn't_. Almost every script fails for me when Git is built with UBSan and sha1dc. It would be nice to make sha1dc respect NO_UNALIGNED_LOADS. But barring that, we should probably default to BLK_SHA1 when we see SANITIZE=undefined. -Peff
disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29
On Mon, Mar 11, 2019 at 11:00:25AM +0900, Junio C Hamano wrote: > Jeffrey Walton writes: > > > I think this is the patch for sha1dc/sha1.c . It stops using unaligned > > accesses by default, but still honors SHA1DC_FORCE_UNALIGNED_ACCESS > > for those who want it. Folks who want the undefined behavior have to > > do something special. > > Hmph, I somehow thought that folks who want to stick to the > standard printed on paper penalizing what practicaly works well in > the real world would be the one doing extra things. Unfortunately, I don't think sha1dc currently supports #defines in that direction. The only logic is "if we are on intel, do unaligned loads" and "even if we are not on intel, do it anyway". There is no "even if we are on intel, do not do unaligned loads". I think you'd need something like this: diff --git a/Makefile b/Makefile index 148668368b..705c54dcd8 100644 --- a/Makefile +++ b/Makefile @@ -1194,6 +1194,7 @@ BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE) BASIC_CFLAGS += -fno-omit-frame-pointer ifneq ($(filter undefined,$(SANITIZERS)),) BASIC_CFLAGS += -DNO_UNALIGNED_LOADS +BASIC_CFLAGS += -DSHA1DC_DISALLOW_UNALIGNED_ACCESS endif ifneq ($(filter leak,$(SANITIZERS)),) BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c index df0630bc6d..0bdf80d778 100644 --- a/sha1dc/sha1.c +++ b/sha1dc/sha1.c @@ -124,9 +124,11 @@ #endif /*ENDIANNESS SELECTION*/ +#ifndef SHA1DC_DISALLOW_UNALIGNED_ACCESS #if defined(SHA1DC_FORCE_UNALIGNED_ACCESS) || defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR) #define SHA1DC_ALLOW_UNALIGNED_ACCESS #endif /*UNALIGNMENT DETECTION*/ +#endif #define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n but of course we cannot touch sha1dc/*, because we might actually be using the submodule copy instead. And AFAIK there is no good way to modify the submodule-provided content as part of the build. Why do we even have the submodule again? ;P I guess the same would be true for DC_SHA1_EXTERNAL, too, though. So anyway, I think this needs a patch to the upstream sha1dc project. -Peff
[PATCH v7 0/8] Fix scissors bug during conflict
This is a complete replacement for dl/merge-cleanup-scissors-fix. Previous discussion on the cherry-pick/revert changes can be found here[1]. Changes since revert/cherry-pick v3: * Rebased on top of latest master * Reordered and squashed patches * Added populate_opts_cb and save_opts to save default_msg_cleanup at Phillip's suggestion [1]: https://public-inbox.org/git/cover.1551867827.git.liu.den...@gmail.com/T/#u Denton Liu (8): t7600: clean up 'merge --squash c3 with c7' test t3507: cleanup space after redirection operators commit: extract cleanup_mode functions to sequencer sequencer.c: remove duplicate code merge: cleanup messages like commit merge: add scissors line on merge conflict sequencer.c: define get_config_from_cleanup cherry-pick/revert: add scissors line on merge conflict Documentation/git-cherry-pick.txt | 7 ++ Documentation/git-revert.txt | 7 ++ Documentation/merge-options.txt | 7 ++ builtin/commit.c | 45 +-- builtin/merge.c | 40 -- builtin/pull.c| 6 ++ builtin/rebase--interactive.c | 2 +- builtin/revert.c | 5 ++ sequencer.c | 102 - sequencer.h | 10 ++- t/t3507-cherry-pick-conflict.sh | 120 +- t/t7600-merge.sh | 53 - t/t7604-merge-custom-message.sh | 61 +++ wt-status.c | 12 ++- wt-status.h | 1 + 15 files changed, 398 insertions(+), 80 deletions(-) base-commit: 6e0cc6776106079ed4efa0cc9abace4107657abf -- 2.21.0.370.g4fdb13b891
[PATCH v7 2/8] t3507: cleanup space after redirection operators
Signed-off-by: Denton Liu --- t/t3507-cherry-pick-conflict.sh | 34 - 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index 0db166152a..74ff925526 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -88,7 +88,7 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' ' test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' ' pristine_detach initial && - echo foo > foo && + echo foo >foo && test_must_fail git cherry-pick base && test_must_fail git rev-parse --verify CHERRY_PICK_HEAD ' @@ -96,7 +96,7 @@ test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' ' test_expect_success \ 'cherry-pick --strategy=resolve w/dirty tree does not set CHERRY_PICK_HEAD' ' pristine_detach initial && - echo foo > foo && + echo foo >foo && test_must_fail git cherry-pick --strategy=resolve base && test_must_fail git rev-parse --verify CHERRY_PICK_HEAD ' @@ -175,23 +175,23 @@ test_expect_success 'failed cherry-pick registers participants in index' ' git ls-files --stage foo && git checkout picked -- foo && git ls-files --stage foo - } > stages && + } >stages && sed " 1 s/ 0 / 1 / 2 s/ 0 / 2 / 3 s/ 0 / 3 / - " < stages > expected && + " expected && git read-tree -u --reset HEAD && test_must_fail git cherry-pick picked && - git ls-files --stage --unmerged > actual && + git ls-files --stage --unmerged >actual && test_cmp expected actual ' test_expect_success 'failed cherry-pick describes conflict in work tree' ' pristine_detach initial && - cat <<-EOF > expected && + cat <<-EOF >expected && <<< HEAD a === @@ -201,14 +201,14 @@ test_expect_success 'failed cherry-pick describes conflict in work tree' ' test_must_fail git cherry-pick picked && - sed "s/[a-f0-9]*\.\.\./objid/" foo > actual && + sed "s/[a-f0-9]*\.\.\./objid/" foo >actual && test_cmp expected actual ' test_expect_success 'diff3 -m style' ' pristine_detach initial && git config merge.conflictstyle diff3 && - cat <<-EOF > expected && + cat <<-EOF >expected && <<< HEAD a ||| parent of objid picked @@ -220,14 +220,14 @@ test_expect_success 'diff3 -m style' ' test_must_fail git cherry-pick picked && - sed "s/[a-f0-9]*\.\.\./objid/" foo > actual && + sed "s/[a-f0-9]*\.\.\./objid/" foo >actual && test_cmp expected actual ' test_expect_success 'revert also handles conflicts sanely' ' git config --unset merge.conflictstyle && pristine_detach initial && - cat <<-EOF > expected && + cat <<-EOF >expected && <<< HEAD a === @@ -241,24 +241,24 @@ test_expect_success 'revert also handles conflicts sanely' ' git ls-files --stage foo && git checkout base -- foo && git ls-files --stage foo - } > stages && + } >stages && sed " 1 s/ 0 / 1 / 2 s/ 0 / 2 / 3 s/ 0 / 3 / - " < stages > expected-stages && + " expected-stages && git read-tree -u --reset HEAD && head=$(git rev-parse HEAD) && test_must_fail git revert picked && newhead=$(git rev-parse HEAD) && - git ls-files --stage --unmerged > actual-stages && + git ls-files --stage --unmerged >actual-stages && test "$head" = "$newhead" && test_must_fail git update-index --refresh -q && test_must_fail git diff-index --exit-code HEAD && test_cmp expected-stages actual-stages && - sed "s/[a-f0-9]*\.\.\./objid/" foo > actual && + sed "s/[a-f0-9]*\.\.\./objid/" foo >actual && test_cmp expected actual ' @@ -284,7 +284,7 @@ test_expect_success 'revert --no-commit sets REVERT_HEAD' ' test_expect_success 'revert w/dirty tree does not set REVERT_HEAD' ' pristine_detach base && - echo foo > foo && + echo foo >foo && test_must_fail git revert base && test_must_fail git rev-parse --verify CHERRY_PICK_HEAD && test_must_fail git rev-parse --verify REVERT_HEAD @@ -319,7 +319,7 @@ test_expect_success 'failed commit does not clear REVERT_HEAD' ' test_expect_success 'revert conflict, diff3 -m style' ' pristine_detach initial && git config merge.conflictstyle diff3 && - cat <<-EOF > expected && + cat <<-EOF >expected && <<< HEAD a ||| objid picked @@ -331,7 +331,7 @@ test_expect_success 'rever
[PATCH v7 1/8] t7600: clean up 'merge --squash c3 with c7' test
This cleans up the original test by removing some unnecessary braces and
removing a pipe.
Helped-by: SZEDER Gábor
Signed-off-by: Denton Liu
---
t/t7600-merge.sh | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 106148254d..d879efd330 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -233,8 +233,7 @@ test_expect_success 'merge --squash c3 with c7' '
cat result.9z >file &&
git commit --no-edit -a &&
- {
- cat <<-EOF
+ cat >expect <<-EOF &&
Squashed commit of the following:
$(git show -s c7)
@@ -242,8 +241,8 @@ test_expect_success 'merge --squash c3 with c7' '
# Conflicts:
# file
EOF
- } >expect &&
- git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
+ git cat-file commit HEAD >tmp &&
+ sed -e '1,/^$/d' actual &&
test_cmp expect actual
'
--
2.21.0.370.g4fdb13b891
[PATCH v7 4/8] sequencer.c: remove duplicate code
Since we implemented get_cleanup_mode, we had some duplicate code in
git_sequencer_config which essentially performed the same operations.
Refactor git_sequencer_config to take advantage of the logic already in
get_cleanup_mode.
Note that we had to introduce a separate argument to get_cleanup_mode
indicating whether to die or not on an invalid value. This is because if
we are parsing a config, we do not want to die but instead, we only want
to warn the user, whereas if we are parsing a command-line option, we
would like to actually die.
Finally, this is almost a no-op refactor but not quite. Previously, in
the case that an invalid value is presented, default_msg_cleanup would
not be set. We change the behaviour so that default_msg_cleanup will now
take on the value as if "default" were provided as the cleanup_arg.
Signed-off-by: Denton Liu
---
builtin/commit.c | 2 +-
sequencer.c | 20 +++-
sequencer.h | 2 +-
3 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 0df15e4851..81e3bd21ca 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1172,7 +1172,7 @@ static int parse_and_validate_options(int argc, const
char *argv[],
die(_("Only one of --include/--only/--all/--interactive/--patch
can be used."));
if (argc == 0 && (also || (only && !amend && !allow_empty)))
die(_("No paths with --include/--only does not make sense."));
- cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
+ cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor, 1);
handle_untracked_files_arg(s);
diff --git a/sequencer.c b/sequencer.c
index 224c823b43..612621f221 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -172,17 +172,7 @@ static int git_sequencer_config(const char *k, const char
*v, void *cb)
if (status)
return status;
- if (!strcmp(s, "verbatim"))
- opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
- else if (!strcmp(s, "whitespace"))
- opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
- else if (!strcmp(s, "strip"))
- opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
- else if (!strcmp(s, "scissors"))
- opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
- else
- warning(_("invalid commit message cleanup mode '%s'"),
- s);
+ opts->default_msg_cleanup = get_cleanup_mode(s, 0, 0);
free((char *)s);
return status;
@@ -512,7 +502,7 @@ static int fast_forward_to(struct repository *r,
}
enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
- int use_editor)
+ int use_editor, int die_on_error)
{
if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
return use_editor ? COMMIT_MSG_CLEANUP_ALL :
@@ -526,7 +516,11 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char
*cleanup_arg,
else if (!strcmp(cleanup_arg, "scissors"))
return use_editor ? COMMIT_MSG_CLEANUP_SCISSORS :
COMMIT_MSG_CLEANUP_SPACE;
- else
+ else if (!die_on_error) {
+ warning(_("Invalid cleanup mode %s, falling back to default"),
cleanup_arg);
+ return use_editor ? COMMIT_MSG_CLEANUP_ALL :
+ COMMIT_MSG_CLEANUP_SPACE;
+ } else
die(_("Invalid cleanup mode %s"), cleanup_arg);
}
diff --git a/sequencer.h b/sequencer.h
index eb9bd97ef3..e7908f558e 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -117,7 +117,7 @@ void append_signoff(struct strbuf *msgbuf, size_t
ignore_footer, unsigned flag);
void append_conflicts_hint(struct index_state *istate, struct strbuf *msgbuf);
enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
- int use_editor);
+ int use_editor, int die_on_error);
void cleanup_message(struct strbuf *msgbuf,
enum commit_msg_cleanup_mode cleanup_mode, int verbose);
--
2.21.0.370.g4fdb13b891
[PATCH v7 5/8] merge: cleanup messages like commit
This change allows git-merge messages to be cleaned up with the
commit.cleanup configuration or --cleanup option, just like how
git-commit does it.
We also give git-pull the passthrough option of --cleanup so that it can
also take advantage of this change.
Finally, add testing to ensure that messages are properly cleaned up.
Note that some newlines that were added to the commit message were
removed so that if a file were read via -F, it would be copied
faithfully.
Signed-off-by: Denton Liu
---
Documentation/merge-options.txt | 5 +++
builtin/merge.c | 31 +
builtin/pull.c | 6
t/t7604-merge-custom-message.sh | 61 +
wt-status.c | 12 +--
wt-status.h | 1 +
6 files changed, 106 insertions(+), 10 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 92a7d936c1..646100ea9a 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -32,6 +32,11 @@ they run `git merge`. To make it easier to adjust such
scripts to the
updated behaviour, the environment variable `GIT_MERGE_AUTOEDIT` can be
set to `no` at the beginning of them.
+--cleanup=::
+ This option determines how the merge message will be cleaned up
+ before commiting or being passed on. See linkgit:git-commit[1] for more
+ details.
+
--ff::
When the merge resolves as a fast-forward, only update the branch
pointer, without creating a merge commit. This is the default
diff --git a/builtin/merge.c b/builtin/merge.c
index 5ce8946d39..4f5fcf5ce9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -38,6 +38,7 @@
#include "tag.h"
#include "alias.h"
#include "commit-reach.h"
+#include "wt-status.h"
#define DEFAULT_TWOHEAD (1<<0)
#define DEFAULT_OCTOPUS (1<<1)
@@ -98,6 +99,9 @@ enum ff_type {
static enum ff_type fast_forward = FF_ALLOW;
+static const char *cleanup_arg;
+static enum commit_msg_cleanup_mode cleanup_mode;
+
static int option_parse_message(const struct option *opt,
const char *arg, int unset)
{
@@ -249,6 +253,7 @@ static struct option builtin_merge_options[] = {
N_("perform a commit if the merge succeeds (default)")),
OPT_BOOL('e', "edit", &option_edit,
N_("edit message before committing")),
+ OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip
spaces and #comments from message")),
OPT_SET_INT(0, "ff", &fast_forward, N_("allow fast-forward (default)"),
FF_ALLOW),
OPT_SET_INT_F(0, "ff-only", &fast_forward,
N_("abort if fast-forward is not possible"),
@@ -612,6 +617,8 @@ static int git_merge_config(const char *k, const char *v,
void *cb)
return git_config_string(&pull_twohead, k, v);
else if (!strcmp(k, "pull.octopus"))
return git_config_string(&pull_octopus, k, v);
+ else if (!strcmp(k, "commit.cleanup"))
+ return git_config_string(&cleanup_arg, k, v);
else if (!strcmp(k, "merge.renormalize"))
option_renormalize = git_config_bool(k, v);
else if (!strcmp(k, "merge.ff")) {
@@ -797,23 +804,32 @@ static void abort_commit(struct commit_list *remoteheads,
const char *err_msg)
exit(1);
}
+static const char comment_line_explanation[] =
+N_("Lines starting with '%c' will be ignored.\n");
+
static const char merge_editor_comment[] =
N_("Please enter a commit message to explain why this merge is necessary,\n"
"especially if it merges an updated upstream into a topic branch.\n"
"\n"
- "Lines starting with '%c' will be ignored, and an empty message aborts\n"
- "the commit.\n");
+ "An empty message aborts the commit.\n");
static void write_merge_heads(struct commit_list *);
static void prepare_to_commit(struct commit_list *remoteheads)
{
struct strbuf msg = STRBUF_INIT;
strbuf_addbuf(&msg, &merge_msg);
- strbuf_addch(&msg, '\n');
if (squash)
BUG("the control must not reach here under --squash");
- if (0 < option_edit)
- strbuf_commented_addf(&msg, _(merge_editor_comment),
comment_line_char);
+ if (0 < option_edit) {
+ strbuf_addch(&msg, '\n');
+ if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
+ wt_status_append_cut_line(&msg);
+ else
+ strbuf_commented_addf(&msg,
_(comment_line_explanation), comment_line_char);
+
+ strbuf_commented_addf(&msg, "\n");
+ strbuf_commented_addf(&msg, _(merge_editor_comment));
+ }
if (signoff)
append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
write_merge_heads(remoteheads);
@@ -832,7 +848,7 @@ static void prepare_to_commit(struct commit_list
*remoteheads)
abort_commit
[PATCH v7 7/8] sequencer.c: define get_config_from_cleanup
Define a function which allows us to get the string configuration value
of a enum commit_msg_cleanup_mode. This is done by refactoring
get_cleanup_mode such that it uses a lookup table to find the mappings
between string and enum and then using the same LUT in reverse to define
get_config_from_cleanup.
Signed-off-by: Denton Liu
---
sequencer.c | 67 +
sequencer.h | 1 +
2 files changed, 53 insertions(+), 15 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 612621f221..5d94e2c865 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -160,6 +160,22 @@ static GIT_PATH_FUNC(rebase_path_strategy_opts,
"rebase-merge/strategy_opts")
static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate,
"rebase-merge/allow_rerere_autoupdate")
static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec,
"rebase-merge/reschedule-failed-exec")
+struct cleanup_config_mapping {
+const char *config_value;
+enum commit_msg_cleanup_mode editor_cleanup;
+enum commit_msg_cleanup_mode no_editor_cleanup;
+};
+
+/* note that we assume that cleanup_config_mapping[0] contains the default
settings */
+struct cleanup_config_mapping cleanup_config_mappings[] = {
+ { "default", COMMIT_MSG_CLEANUP_ALL, COMMIT_MSG_CLEANUP_SPACE },
+ { "verbatim", COMMIT_MSG_CLEANUP_NONE, COMMIT_MSG_CLEANUP_NONE },
+ { "whitespace", COMMIT_MSG_CLEANUP_SPACE, COMMIT_MSG_CLEANUP_SPACE },
+ { "strip", COMMIT_MSG_CLEANUP_ALL, COMMIT_MSG_CLEANUP_ALL },
+ { "scissors", COMMIT_MSG_CLEANUP_SCISSORS, COMMIT_MSG_CLEANUP_SPACE },
+ { NULL, 0, 0 }
+};
+
static int git_sequencer_config(const char *k, const char *v, void *cb)
{
struct replay_opts *opts = cb;
@@ -504,26 +520,42 @@ static int fast_forward_to(struct repository *r,
enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
int use_editor, int die_on_error)
{
- if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
- return use_editor ? COMMIT_MSG_CLEANUP_ALL :
- COMMIT_MSG_CLEANUP_SPACE;
- else if (!strcmp(cleanup_arg, "verbatim"))
- return COMMIT_MSG_CLEANUP_NONE;
- else if (!strcmp(cleanup_arg, "whitespace"))
- return COMMIT_MSG_CLEANUP_SPACE;
- else if (!strcmp(cleanup_arg, "strip"))
- return COMMIT_MSG_CLEANUP_ALL;
- else if (!strcmp(cleanup_arg, "scissors"))
- return use_editor ? COMMIT_MSG_CLEANUP_SCISSORS :
- COMMIT_MSG_CLEANUP_SPACE;
- else if (!die_on_error) {
+ struct cleanup_config_mapping *default_mapping =
&cleanup_config_mappings[0];
+ struct cleanup_config_mapping *current_mapping;
+
+ if (!cleanup_arg) {
+ return use_editor ? default_mapping->editor_cleanup :
+ default_mapping->no_editor_cleanup;
+ }
+
+ for (current_mapping = cleanup_config_mappings;
current_mapping->config_value; current_mapping++) {
+ if (!strcmp(cleanup_arg, current_mapping->config_value)) {
+ return use_editor ? current_mapping->editor_cleanup :
+ current_mapping->no_editor_cleanup;
+ }
+ }
+
+ if (!die_on_error) {
warning(_("Invalid cleanup mode %s, falling back to default"),
cleanup_arg);
- return use_editor ? COMMIT_MSG_CLEANUP_ALL :
- COMMIT_MSG_CLEANUP_SPACE;
+ return use_editor ? default_mapping->editor_cleanup :
+ default_mapping->no_editor_cleanup;
} else
die(_("Invalid cleanup mode %s"), cleanup_arg);
}
+const char *get_config_from_cleanup(enum commit_msg_cleanup_mode cleanup_mode)
+{
+ struct cleanup_config_mapping *current_mapping;
+
+ for (current_mapping = &cleanup_config_mappings[1];
current_mapping->config_value; current_mapping++) {
+ if (cleanup_mode == current_mapping->editor_cleanup) {
+ return current_mapping->config_value;
+ }
+ }
+
+ BUG(_("invalid cleanup_mode provided"));
+}
+
void append_conflicts_hint(struct index_state *istate,
struct strbuf *msgbuf)
{
@@ -2350,6 +2382,8 @@ static int populate_opts_cb(const char *key, const char
*value, void *data)
opts->allow_rerere_auto =
git_config_bool_or_int(key, value, &error_flag) ?
RERERE_AUTOUPDATE : RERERE_NOAUTOUPDATE;
+ else if (!strcmp(key, "options.default-msg-cleanup"))
+ opts->default_msg_cleanup = get_cleanup_mode(value, 1, 1);
else
return error(_("invalid key: %s"), key);
@@ -2754,6 +2788,9 @@ static int save_opts(struct replay_opts *opts)
res |= git_config_set_in_file_gently(opts_file,
"opt
[PATCH v7 8/8] cherry-pick/revert: add scissors line on merge conflict
Fix a bug where the scissors line is placed after the Conflicts:
section, in the case where a merge conflict occurs and
commit.cleanup = scissors.
Signed-off-by: Denton Liu
---
Documentation/git-cherry-pick.txt | 7 +++
Documentation/git-revert.txt | 7 +++
builtin/merge.c | 9 +---
builtin/rebase--interactive.c | 2 +-
builtin/revert.c | 5 ++
sequencer.c | 22 +---
sequencer.h | 3 +-
t/t3507-cherry-pick-conflict.sh | 86 +++
8 files changed, 123 insertions(+), 18 deletions(-)
diff --git a/Documentation/git-cherry-pick.txt
b/Documentation/git-cherry-pick.txt
index b8cfeec67e..b4ff8e136d 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -57,6 +57,13 @@ OPTIONS
With this option, 'git cherry-pick' will let you edit the commit
message prior to committing.
+--cleanup=::
+ This option determines how the commit message will be cleaned up before
+ being passed on. See linkgit:git-commit[1] for more details. In
+ addition, if the '' is given a value of `scissors`, scissors will
+ be appended to MERGE_MSG before being passed on in the case of a
+ conflict.
+
-x::
When recording the commit, append a line that says
"(cherry picked from commit ...)" to the original commit
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 837707a8fd..bd4ad395a9 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -66,6 +66,13 @@ more details.
With this option, 'git revert' will not start the commit
message editor.
+--cleanup=::
+ This option determines how the commit message will be cleaned up before
+ being passed on. See linkgit:git-commit[1] for more details. In
+ addition, if the '' is given a value of `scissors`, scissors will
+ be appended to MERGE_MSG before being passed on in the case of a
+ conflict.
+
-n::
--no-commit::
Usually the command automatically creates some commits with
diff --git a/builtin/merge.c b/builtin/merge.c
index b620d4bfcb..671d6a19a4 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -923,14 +923,7 @@ static int suggest_conflicts(void)
* Thus, we will get the cleanup mode which is returned when we _are_
using
* an editor.
*/
- if (get_cleanup_mode(cleanup_arg, 1, 1) == COMMIT_MSG_CLEANUP_SCISSORS)
{
- fputc('\n', fp);
- wt_status_add_cut_line(fp);
- /* comments out the newline from append_conflicts_hint */
- fputc(comment_line_char, fp);
- }
-
- append_conflicts_hint(&the_index, &msgbuf);
+ append_conflicts_hint(&the_index, &msgbuf,
get_cleanup_mode(cleanup_arg, 1, 1));
fputs(msgbuf.buf, fp);
strbuf_release(&msgbuf);
fclose(fp);
diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index 888390f911..cf2151b271 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -199,10 +199,10 @@ int cmd_rebase__interactive(int argc, const char **argv,
const char *prefix)
OPT_END()
};
+ opts.action = REPLAY_INTERACTIVE_REBASE;
sequencer_init_config(&opts);
git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
- opts.action = REPLAY_INTERACTIVE_REBASE;
opts.allow_ff = 1;
opts.allow_empty = 1;
diff --git a/builtin/revert.c b/builtin/revert.c
index a47b53ceaf..e8168e0214 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -96,11 +96,13 @@ static int run_sequencer(int argc, const char **argv,
struct replay_opts *opts)
{
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
+ const char *cleanup_arg = NULL;
int cmd = 0;
struct option base_options[] = {
OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick
sequence"), 'q'),
OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or
cherry-pick sequence"), 'c'),
OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick
sequence"), 'a'),
+ OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how
to strip spaces and #comments from message")),
OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't
automatically commit")),
OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit
message")),
OPT_NOOP_NOARG('r', NULL),
@@ -137,6 +139,9 @@ static int run_sequencer(int argc, const char **argv,
struct replay_opts *opts)
if (opts->keep_redundant_commits)
opts->allow_empty = 1;
+ if (cleanup_arg)
+ opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1, 1);
+
/* Check for incompatible command line arguments */
[PATCH v7 3/8] commit: extract cleanup_mode functions to sequencer
Signed-off-by: Denton Liu
---
builtin/commit.c | 25 ++---
sequencer.c | 29 +
sequencer.h | 6 ++
3 files changed, 37 insertions(+), 23 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index f17537474a..0df15e4851 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1172,24 +1172,7 @@ static int parse_and_validate_options(int argc, const
char *argv[],
die(_("Only one of --include/--only/--all/--interactive/--patch
can be used."));
if (argc == 0 && (also || (only && !amend && !allow_empty)))
die(_("No paths with --include/--only does not make sense."));
- if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
- cleanup_mode = use_editor ? COMMIT_MSG_CLEANUP_ALL :
- COMMIT_MSG_CLEANUP_SPACE;
- else if (!strcmp(cleanup_arg, "verbatim"))
- cleanup_mode = COMMIT_MSG_CLEANUP_NONE;
- else if (!strcmp(cleanup_arg, "whitespace"))
- cleanup_mode = COMMIT_MSG_CLEANUP_SPACE;
- else if (!strcmp(cleanup_arg, "strip"))
- cleanup_mode = COMMIT_MSG_CLEANUP_ALL;
- else if (!strcmp(cleanup_arg, "scissors"))
- cleanup_mode = use_editor ? COMMIT_MSG_CLEANUP_SCISSORS :
- COMMIT_MSG_CLEANUP_SPACE;
- /*
-* Please update _git_commit() in git-completion.bash when you
-* add new options.
-*/
- else
- die(_("Invalid cleanup mode %s"), cleanup_arg);
+ cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
handle_untracked_files_arg(s);
@@ -1626,11 +1609,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
die(_("could not read commit message: %s"),
strerror(saved_errno));
}
- if (verbose || /* Truncate the message just before the diff, if any. */
- cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
- strbuf_setlen(&sb, wt_status_locate_end(sb.buf, sb.len));
- if (cleanup_mode != COMMIT_MSG_CLEANUP_NONE)
- strbuf_stripspace(&sb, cleanup_mode == COMMIT_MSG_CLEANUP_ALL);
+ cleanup_message(&sb, cleanup_mode, verbose);
if (message_is_empty(&sb, cleanup_mode) && !allow_empty_message) {
rollback_index_files();
diff --git a/sequencer.c b/sequencer.c
index 95dda23eee..224c823b43 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -511,6 +511,25 @@ static int fast_forward_to(struct repository *r,
return 0;
}
+enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
+ int use_editor)
+{
+ if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
+ return use_editor ? COMMIT_MSG_CLEANUP_ALL :
+ COMMIT_MSG_CLEANUP_SPACE;
+ else if (!strcmp(cleanup_arg, "verbatim"))
+ return COMMIT_MSG_CLEANUP_NONE;
+ else if (!strcmp(cleanup_arg, "whitespace"))
+ return COMMIT_MSG_CLEANUP_SPACE;
+ else if (!strcmp(cleanup_arg, "strip"))
+ return COMMIT_MSG_CLEANUP_ALL;
+ else if (!strcmp(cleanup_arg, "scissors"))
+ return use_editor ? COMMIT_MSG_CLEANUP_SCISSORS :
+ COMMIT_MSG_CLEANUP_SPACE;
+ else
+ die(_("Invalid cleanup mode %s"), cleanup_arg);
+}
+
void append_conflicts_hint(struct index_state *istate,
struct strbuf *msgbuf)
{
@@ -1013,6 +1032,16 @@ static int rest_is_empty(const struct strbuf *sb, int
start)
return 1;
}
+void cleanup_message(struct strbuf *msgbuf,
+ enum commit_msg_cleanup_mode cleanup_mode, int verbose)
+{
+ if (verbose || /* Truncate the message just before the diff, if any. */
+ cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
+ strbuf_setlen(msgbuf, wt_status_locate_end(msgbuf->buf,
msgbuf->len));
+ if (cleanup_mode != COMMIT_MSG_CLEANUP_NONE)
+ strbuf_stripspace(msgbuf, cleanup_mode ==
COMMIT_MSG_CLEANUP_ALL);
+}
+
/*
* Find out if the message in the strbuf contains only whitespace and
* Signed-off-by lines.
diff --git a/sequencer.h b/sequencer.h
index 4d505b3590..eb9bd97ef3 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -116,6 +116,12 @@ int rearrange_squash(struct repository *r);
void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned
flag);
void append_conflicts_hint(struct index_state *istate, struct strbuf *msgbuf);
+enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
+ int use_editor);
+
+void cleanup_message(struct strbuf *msgbuf,
+ enum commit_msg_cleanup_mode cleanup_mode, int verbose);
+
int message_is_empty(const struct strbuf *sb,
enum commit_msg_cleanup_mode cleanup_mode);
int template_untouched(const struct strbuf *sb, const char *template_file,
--
2.21.0.3
[PATCH v7 6/8] merge: add scissors line on merge conflict
This fixes a bug where the scissors line is placed after the Conflicts:
section, in the case where a merge conflict occurs and
commit.cleanup = scissors.
Next, if commit.cleanup = scissors is specified, don't produce a
scissors line in commit if one already exists in the MERGE_MSG file.
Signed-off-by: Denton Liu
---
Documentation/merge-options.txt | 4 ++-
builtin/commit.c| 20 ++
builtin/merge.c | 14 ++
t/t7600-merge.sh| 46 +
4 files changed, 78 insertions(+), 6 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 646100ea9a..c7b889d6f5 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -35,7 +35,9 @@ set to `no` at the beginning of them.
--cleanup=::
This option determines how the merge message will be cleaned up
before commiting or being passed on. See linkgit:git-commit[1] for more
- details.
+ details. In addition, if the '' is given a value of `scissors`,
+ scissors will be appended to MERGE_MSG before being passed on in the
+ case of a merge conflict.
--ff::
When the merge resolves as a fast-forward, only update the branch
diff --git a/builtin/commit.c b/builtin/commit.c
index 81e3bd21ca..d8c4626a68 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -668,6 +668,7 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
const char *hook_arg2 = NULL;
int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
int old_display_comment_prefix;
+ int merge_contains_scissors = 0;
/* This checks and barfs if author is badly specified */
determine_author_info(author_ident);
@@ -728,6 +729,8 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
strbuf_addbuf(&sb, &message);
hook_arg1 = "message";
} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
+ size_t merge_msg_start;
+
/*
* prepend SQUASH_MSG here if it exists and a
* "merge --squash" was originally performed
@@ -738,8 +741,14 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
hook_arg1 = "squash";
} else
hook_arg1 = "merge";
+
+ merge_msg_start = sb.len;
if (strbuf_read_file(&sb, git_path_merge_msg(the_repository),
0) < 0)
die_errno(_("could not read MERGE_MSG"));
+
+ if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
+ wt_status_locate_end(sb.buf + merge_msg_start, sb.len -
merge_msg_start) < sb.len - merge_msg_start)
+ merge_contains_scissors = 1;
} else if (!stat(git_path_squash_msg(the_repository), &statbuf)) {
if (strbuf_read_file(&sb, git_path_squash_msg(the_repository),
0) < 0)
die_errno(_("could not read SQUASH_MSG"));
@@ -807,7 +816,8 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
struct ident_split ci, ai;
if (whence != FROM_COMMIT) {
- if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
+ if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
+ !merge_contains_scissors)
wt_status_add_cut_line(s->fp);
status_printf_ln(s, GIT_COLOR_NORMAL,
whence == FROM_MERGE
@@ -832,10 +842,10 @@ static int prepare_to_commit(const char *index_file,
const char *prefix,
_("Please enter the commit message for your
changes."
" Lines starting\nwith '%c' will be ignored,
and an empty"
" message aborts the commit.\n"),
comment_line_char);
- else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
-whence == FROM_COMMIT)
- wt_status_add_cut_line(s->fp);
- else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
+ else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
+ if (whence == FROM_COMMIT && !merge_contains_scissors)
+ wt_status_add_cut_line(s->fp);
+ } else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
status_printf(s, GIT_COLOR_NORMAL,
_("Please enter the commit message for your
changes."
" Lines starting\n"
diff --git a/builtin/merge.c b/builtin/merge.c
index 4f5fcf5ce9..b620d4bfcb 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -916,6 +916,20 @@ static int suggest_conflicts(void)
filename = git_p
Re: [PATCH v2] line-log: suppress diff output with "-s"
On Fri, Mar 08, 2019 at 07:44:22PM +0100, Johannes Schindelin wrote:
> > Do we want to do something like this:
> >
> > diff --git a/revision.c b/revision.c
> > index eb8e51bc63..a1b4fe2aa6 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -2689,6 +2689,10 @@ int setup_revisions(int argc, const char **argv,
> > struct rev_info *revs, struct s
> > if (revs->first_parent_only && revs->bisect)
> > die(_("--first-parent is incompatible with --bisect"));
> >
> > + if (revs->line_level_traverse &&
> > + (revs->diffopt.output_format &
> > ~(DIFF_FORMAT_PATCH|DIFF_FORMAT_NO_OUTPUT)))
> > + die(_("-L does not yet support diff formats besides -p and
> > -s"));
> > +
> > if (revs->expand_tabs_in_log < 0)
> > revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
>
> Since you already have that patch, why not go wild and apply it, too? ;-)
OK, then. Here it is as a real patch.
> I guess you copy-edited the code from somewhere because you usually do
> leave spaces around the `|`... I don't care, though ;-)
Heh, nope, I wrote it by hand.
I actually wondered if there was a better way to write this. One of the
things that bugs me about the patch is that this policy check is so far
from actual line-log code, which means the two may drift apart over
time. We could put a LINE_LOG_SUPPORTED_FORMATS into line-log.h, but
that's also pretty far from the actual code. We could just make the
output logic in the line-log code something like:
if (NO_OUTPUT)
...do no output...
else if (PATCH || DEFAULT)
...show normal patch...
else
die("unsupported output format");
but it's probably a little friendlier to diagnose the problem up front,
not after we did a bunch of computation. And most of these other
"incompatible" checks in setup_revisions() suffer from the same
maintainability problem. So instead, I went with the code above (with
spaces!) and protected it with some tests.
-- >8 --
Subject: [PATCH] line-log: detect unsupported formats
If you use "log -L" with an output format like "--raw" or "--stat",
we'll silently ignore the format and just output the normal patch.
Let's detect and complain about this, which at least tells the user
what's going on.
The tests here aren't exhaustive over the set of all formats, but it
should at least let us know if somebody breaks the format-checking.
Signed-off-by: Jeff King
---
On top of the other patch, naturally.
revision.c | 4
t/t4211-line-log.sh | 10 ++
2 files changed, 14 insertions(+)
diff --git a/revision.c b/revision.c
index eb8e51bc63..cb69a227d5 100644
--- a/revision.c
+++ b/revision.c
@@ -2689,6 +2689,10 @@ int setup_revisions(int argc, const char **argv, struct
rev_info *revs, struct s
if (revs->first_parent_only && revs->bisect)
die(_("--first-parent is incompatible with --bisect"));
+ if (revs->line_level_traverse &&
+ (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH |
DIFF_FORMAT_NO_OUTPUT)))
+ die(_("-L does not yet support diff formats besides -p and
-s"));
+
if (revs->expand_tabs_in_log < 0)
revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index c9f2036f68..1db7bd0f59 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -122,4 +122,14 @@ test_expect_success '-s shows only line-log commits' '
test_cmp expect actual
'
+test_expect_success '-p shows the default patch output' '
+ git log -L1,24:b.c >expect &&
+ git log -L1,24:b.c -p >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--raw is forbidden' '
+ test_must_fail git log -L1,24:b.c --raw
+'
+
test_done
--
2.21.0.787.gbeacff058d
Re: [PATCH 0/2] fix spurious space after linkgit, now in *.html
On Mon, 11 Mar 2019 at 04:02, Junio C Hamano wrote: > > Martin Ågren writes: > > > Since this has hit "next", here are two patches on top to address this. > > > > Sorry about this. > > Thanks. > > If it makes it easier, we can eject ma/asciidoctor-fixes out of > 'next' when we rebuild 'next'. I actually am tempted to start > 'next' from empty for this round after merging some obviously > correct ones that are there to the 'master' branch. Ok, then I'll take that offer. I'll squash these two patches into the original series. Thanks. Martin
Re: [PATCH 0/2] fix spurious space after linkgit, now in *.html
On Mon, 11 Mar 2019 at 03:59, Jeff King wrote: > > On Sun, Mar 10, 2019 at 04:14:22PM +0100, Martin Ågren wrote: > > > [1] I could have sworn I checked the html docs and saw that they didn't > > have this extra space. Looking at git-scm.com again reveals that it's > > not there. Huh. Seems like the site's html-rendering doesn't go through our > > Makefile at all and handles "linkgit:" on its own: > > https://github.com/git/git-scm.com/blob/master/script/doc_importer > > Correct. It significantly predates any support for asciidoctor in our > Makefile, and needs to do some custom ruby-level tweaking (though > possibly that could be cleaned up these days). > > The doc_importer script you found, though, is not part of that process. > I _think_ it's just leftover cruft. The actual import and conversion > happens as part of the rake task in: > > https://github.com/git/git-scm.com/blob/master/lib/tasks/index.rake > > (Not that any of this matters for your series; just sprinkling some fun > facts into the conversation). Thanks for this background info, and for the correct link. :-) Martin
Re: [PATCH v7 5/8] merge: cleanup messages like commit
On Sun, Mar 10, 2019 at 11:42 PM Denton Liu wrote:
> This change allows git-merge messages to be cleaned up with the
> commit.cleanup configuration or --cleanup option, just like how
> git-commit does it.
>
> We also give git-pull the passthrough option of --cleanup so that it can
> also take advantage of this change.
>
> Finally, add testing to ensure that messages are properly cleaned up.
> Note that some newlines that were added to the commit message were
> removed so that if a file were read via -F, it would be copied
> faithfully.
>
> Signed-off-by: Denton Liu
> ---
> diff --git a/t/t7604-merge-custom-message.sh b/t/t7604-merge-custom-message.sh
> @@ -47,4 +47,65 @@ test_expect_success 'merge --log appends to custom
> message' '
> +test_expect_success 'cleanup commit messages (verbatim option)' '
> + git reset --hard c1 &&
> + git merge --cleanup=verbatim -F expect c2 &&
> + git cat-file -p HEAD |sed -e "1,/^\$/d" >actual &&
An earlier patch in this series "fixed" a test with a Git command
upstream of a pipe. Yet, this test adds such an instance. (Also,
style: add space after '|'.)
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'cleanup commit messages (whitespace option)' '
> + git reset --hard c1 &&
> + { echo;echo "# text";echo; } >text &&
Style: add space after semicolon or use &&-chaining inside {...}.
Alternately, less ugly:
test_write_lines "" "# text" "" >text &&
(Or even a here-doc, though the leading and trailing blank lines make
the here-doc ugly.)
> + echo "# text" >expect &&
> + git merge --cleanup=whitespace -F text c2 &&
> + git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
Git upstream pipe.
> + test_cmp expect actual
> +
> +'
Style: drop the blank line before the closing quote.
> +test_expect_success 'cleanup merge messages (scissors option)' '
> + git reset --hard c1 &&
> + cat >text +# to be kept
> +
> + # >8
> +# to be kept, too
> +# >8
> +to be removed
> +# >8
> +to be removed, too
> +EOF
> +
> + cat >expect < +# to be kept
> +
> + # >8
> +# to be kept, too
> +EOF
> + git merge --cleanup=scissors -e -F text c2 &&
> + git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
Git upstream pipe.
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'cleanup commit messages (strip option)' '
> + git reset --hard c1 &&
> + { echo;echo "# text";echo sample;echo; } >text &&
test_write_lines "" "# text" sample "" >text &&
> + echo sample >expect &&
> + git merge --cleanup=strip -F text c2 &&
> + git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
Git upstream pipe.
> + test_cmp expect actual
> +
> +'
Drop blank line before closing quote.
Re: [PATCH v7 6/8] merge: add scissors line on merge conflict
On Sun, Mar 10, 2019 at 11:42 PM Denton Liu wrote: > This fixes a bug where the scissors line is placed after the Conflicts: > section, in the case where a merge conflict occurs and > commit.cleanup = scissors. > > Next, if commit.cleanup = scissors is specified, don't produce a > scissors line in commit if one already exists in the MERGE_MSG file. > > Signed-off-by: Denton Liu > --- > diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh > @@ -246,6 +246,52 @@ test_expect_success 'merge --squash c3 with c7' ' > +test_expect_success 'merge c3 with c7 with commit.cleanup = scissors' ' > + git config commit.cleanup scissors && > + git reset --hard c3 && > + test_must_fail git merge c7 && > + cat result.9z >file && > + git commit --no-edit -a && > + > + cat >expect <<-EOF && Use <<-\EOF here. > + Merge tag '"'"'c7'"'"' > + > + # >8 > + # Do not modify or remove the line above. > + # Everything below it will be ignored. > + # > + # Conflicts: > + # file > + EOF Style: here-doc body is normally indented to the same level as the command which starts the here-doc, so this body is indented too much. Same comments apply to other new test added by this patch. > + git cat-file commit HEAD >tmp && > + sed -e '1,/^$/d' actual && > + test_i18ncmp expect actual > +'
Re: [PATCH v7 7/8] sequencer.c: define get_config_from_cleanup
On Sun, Mar 10, 2019 at 11:42 PM Denton Liu wrote:
> Define a function which allows us to get the string configuration value
> of a enum commit_msg_cleanup_mode. This is done by refactoring
> get_cleanup_mode such that it uses a lookup table to find the mappings
> between string and enum and then using the same LUT in reverse to define
> get_config_from_cleanup.
Aside from a missing 'static', the comments below are mostly style
suggestions to make the new code less noisy. The basic idea is to
reduce the "wordiness" of the code so that the eye can glide over it
more easily, thus allowing the reader to grasp its meaning "at a
glance", without necessarily having to read it attentively. You may or
may not consider the suggestions actionable.
> Signed-off-by: Denton Liu
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -160,6 +160,22 @@ static GIT_PATH_FUNC(rebase_path_strategy_opts,
> "rebase-merge/strategy_opts")
> +struct cleanup_config_mapping {
> +const char *config_value;
> +enum commit_msg_cleanup_mode editor_cleanup;
> +enum commit_msg_cleanup_mode no_editor_cleanup;
> +};
These members are already inside a struct named
"cleanup_config_mapping", so we can drop some of the wordiness from
the member names. For instance:
config --or-- value --or-- val
editor
no_editor
> +/* note that we assume that cleanup_config_mapping[0] contains the default
> settings */
> +struct cleanup_config_mapping cleanup_config_mappings[] = {
> + { "default", COMMIT_MSG_CLEANUP_ALL, COMMIT_MSG_CLEANUP_SPACE },
> + { "verbatim", COMMIT_MSG_CLEANUP_NONE, COMMIT_MSG_CLEANUP_NONE },
> + { "whitespace", COMMIT_MSG_CLEANUP_SPACE, COMMIT_MSG_CLEANUP_SPACE },
> + { "strip", COMMIT_MSG_CLEANUP_ALL, COMMIT_MSG_CLEANUP_ALL },
> + { "scissors", COMMIT_MSG_CLEANUP_SCISSORS, COMMIT_MSG_CLEANUP_SPACE },
> + { NULL, 0, 0 }
> +};
This table should be 'static'.
> @@ -504,26 +520,42 @@ static int fast_forward_to(struct repository *r,
> enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
> int use_editor, int die_on_error)
> {
> + struct cleanup_config_mapping *default_mapping =
> &cleanup_config_mappings[0];
> + struct cleanup_config_mapping *current_mapping;
We can shorten these two variable names to "def" and "p",
respectively, without losing clarity; there are only two variables in
the function, so it's not difficult to remember what they are. More
importantly, the rest of the code becomes considerably shorter,
allowing the eye to glide over it while easily taking in its meaning
rather than having to spend time actively reading it.
> + if (!cleanup_arg) {
> + return use_editor ? default_mapping->editor_cleanup :
> + default_mapping->no_editor_cleanup;
> + }
> +
> + for (current_mapping = cleanup_config_mappings;
> current_mapping->config_value; current_mapping++) {
> + if (!strcmp(cleanup_arg, current_mapping->config_value)) {
> + return use_editor ? current_mapping->editor_cleanup :
> +
> current_mapping->no_editor_cleanup;
> + }
> + }
For instance, with the shorter names, the above loop (while also
dropping unnecessary braces) becomes:
for (p = cleanup_config_mappings; p->val; p++)
if (!strcmp(cleanup_arg, p->val))
return use_editor ? p->editor : p->no_editor;
> + if (!die_on_error) {
> warning(_("Invalid cleanup mode %s, falling back to
> default"), cleanup_arg);
> - return use_editor ? COMMIT_MSG_CLEANUP_ALL :
> - COMMIT_MSG_CLEANUP_SPACE;
> + return use_editor ? default_mapping->editor_cleanup :
> + default_mapping->no_editor_cleanup;
> } else
> die(_("Invalid cleanup mode %s"), cleanup_arg);
> }
Same comments apply to other new code introduced by this patch.
> +const char *get_config_from_cleanup(enum commit_msg_cleanup_mode
> cleanup_mode)
> +{
> + struct cleanup_config_mapping *current_mapping;
> +
> + for (current_mapping = &cleanup_config_mappings[1];
> current_mapping->config_value; current_mapping++) {
> + if (cleanup_mode == current_mapping->editor_cleanup) {
> + return current_mapping->config_value;
> + }
> + }
> +
> + BUG(_("invalid cleanup_mode provided"));
Don't localize BUG() messages; they are intended only for developer
eyes, not end-users. So:
BUG("invalid cleanup_mode provided");
> +}
> diff --git a/sequencer.h b/sequencer.h
> @@ -118,6 +118,7 @@ void append_signoff(struct strbuf *msgbuf, size_t
> ignore_footer, unsigned flag);
> enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
> int use_editor, int die_on_error);
> +const char *get_config_from_cleanup(enum commi
Re: [PATCH v5 1/1] worktree add: sanitize worktree names
Eric Sunshine writes:
>> case 2:
>> + if (last == '.') { /* Refname contains "..". */
>> + if (sanitized)
>> + sanitized->len--; /* collapse ".."
>> to single "." */
>
> I think this needs to be:
>
> strbuf_setlen(sanitized, sanitized->len - 1);
>
> to ensure that NUL-terminator ends up in the correct place if this "."
> is the very last character in 'refname'. (Otherwise, the NUL will
> remain after the second ".", thus ".." won't be collapsed to "." at
> all.)
True. Why doesn't it do the similar "replace with -" it does for
other unfortunate characters, though?
Re: [PATCH v5 1/1] worktree add: sanitize worktree names
Nguyễn Thái Ngọc Duy writes:
> Update 'worktree add' code to remove special characters to follow
> these rules. In the future the user will be able to specify the
> worktree name by themselves if they're not happy with this dumb
> character substitution.
This replaces both of the two patches in v4, and applies to a more
recent tip of 'master' (post 7d0c1f4556).
> diff --git a/refs.c b/refs.c
> index 142888a40a..e9f83018f0 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -72,30 +72,57 @@ static unsigned char refname_disposition[256] = {
> * - it ends with ".lock", or
> * - it contains a "@{" portion
> */
The comment above needs modernizing (see attached at the end).
> case 2:
> - if (last == '.')
> - return -1; /* Refname contains "..". */
> + if (last == '.') { /* Refname contains "..". */
> + if (sanitized)
> + sanitized->len--; /* collapse ".." to
> single "." */
As Eric points out, this needs to be fixed.
I'll use the strbuf_setlen() version suggested by Eric in the
meantime, but "sanitized->buf[sanitized->len-1] = '-'" as done to
everything else in the function may be a better idea, especially
since they'll be able to name the worktree themselves in the future
anyway.
> +
> + if (refname[0] == '.') { /* Component starts with '.'. */
> + if (sanitized)
> + sanitized->buf[component_start] = '-';
... and a dot turns into a dash in some cases anyway.
> + else
> + return -1;
> + }
> if (cp - refname >= LOCK_SUFFIX_LEN &&
> - !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))
> - return -1; /* Refname ends with ".lock". */
> + !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN)) {
> + if (!sanitized)
> + return -1;
> + /* Refname ends with ".lock". */
> + while (strbuf_strip_suffix(sanitized, LOCK_SUFFIX)) {
> + /* try again in case we have .lock.lock */
> + }
No need for {}; just have an empty statment
while (...)
; /* try again ... */
This "strip all .lock repeatedly" made me stop and think a bit; this
will never make the component empty, as the only way for this loop
to make it empty is if we have a string that match "^\(.lock)\*$" in
it, but the first dot would have already been turned into a dash, so
we'll end up with "-lock", which is not empty.
> + }
> return cp - refname;
> }
See below for a possible further polishment.
* The first hunk is not about this patch but a long-standing issue
after the comment was given to this function for a single level
(I do not know or care how it happened--perhaps we had a single
function that verifies multiple levels which later was split into
a caller that loops and this function that checks a single level,
and the comment for the multi-level function was left stale).
* check_refname_component() can now try to sanitize; document it.
* The last hunk is from Eric's comment.
refs.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index e9f83018f0..3a1b2a8c3c 100644
--- a/refs.c
+++ b/refs.c
@@ -63,7 +63,7 @@ static unsigned char refname_disposition[256] = {
* not legal. It is legal if it is something reasonable to have under
* ".git/refs/"; We do not like it if:
*
- * - any path component of it begins with ".", or
+ * - it begins with ".", or
* - it has double dots "..", or
* - it has ASCII control characters, or
* - it has ":", "?", "[", "\", "^", "~", SP, or TAB anywhere, or
@@ -71,6 +71,10 @@ static unsigned char refname_disposition[256] = {
* - it ends with a "/", or
* - it ends with ".lock", or
* - it contains a "@{" portion
+ *
+ * When sanitized is not NULL, instead of rejecting the input refname
+ * as an error, try to come up with a usable replacement for the input
+ * refname in it.
*/
static int check_refname_component(const char *refname, int *flags,
struct strbuf *sanitized)
@@ -95,7 +99,8 @@ static int check_refname_component(const char *refname, int
*flags,
case 2:
if (last == '.') { /* Refname contains "..". */
if (sanitized)
- sanitized->len--; /* collapse ".." to
single "." */
+ /* collapse ".." to single "." */
+ strbuf_setlen(sanitized, sanitized->len
- 1);
else
return -1;
}
Re: [PATCH v7 0/8] Fix scissors bug during conflict
Denton Liu writes: > This is a complete replacement for dl/merge-cleanup-scissors-fix. Thanks; will take a look and requeue.
