Re: [PATCH v3 2/5] pretty: allow showing specific trailers
Anders Waldenborg writes: > Would it feel less inconsistent if it did not set the 'only_trailers' > option? If %(trailers:key=...) did not automatically imply 'only', it would be very consistent. But as I already said, I think it would be less convenient, as I do suspect that those who want specific keys would want to see only those trailers with specific keys. And if we want that convinience, we'd probably want a way to optionally disable that 'only' bit when the user wants to. And... > --- a/Documentation/pretty-formats.txt > +++ b/Documentation/pretty-formats.txt > @@ -228,9 +228,9 @@ endif::git-rev-list[] > ** 'key=': only show trailers with specified key. Matching is done > case-insensitively and trailing colon is optional. If option is > given multiple times trailer lines matching any of the keys are > - shown. Non-trailer lines in the trailer block are also hidden > - (i.e. 'key' implies 'only'). E.g., `%(trailers:key=Reviewed-by)` > - shows trailer lines with key `Reviewed-by`. > + shown. Non-trailer lines in the trailer block are also hidden. > + E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key > + `Reviewed-by`. ... I do not think this change reduces the puzzlement felt by readers who would have wondered how that implied 'only' can be countermanded with the old text. It just makes it look even less explained to them. If we assume that nobody would ever want to mix non-trailers when asking specific keywords, then "them" in the above paragraph would become an empty set, and we do not have to worry about them. I am not sure if Git is still such a small project to allow us rely on such an assumption, though. > ** 'only': omit non-trailer lines from the trailer block. > ** 'unfold': make it behave as if interpret-trailer's `--unfold` > option was given. E.g., `%(trailers:only,unfold)` unfolds and
Re: [PATCH v11 00/22] Convert "git stash" to C builtin
Junio C Hamano writes: > Thomas Gummerer writes: > >> Thanks for your work on this! I have read through the range-diff and >> the new patch of this last round, and this addresses all the comments >> I had on v10 (and some more :)). I consider it >> Reviewed-by: Thomas Gummerer > > Thanks. > > One thing that bothers me is that this seems to have been rebased on > 'master', but as long as we are rebasing, the updated series must > also take into account of the sd/stash-wo-user-name topic, i.e. if > we are rebasing it, it should be rebased on top of the result of > > git checkout -B ps/rebase-in-c master > git merge --no-ff sd/stash-wo-user-name > > I think. https://travis-ci.org/git/git/builds/459619672 would show that this C reimplementation now regresses from the scripted version due to lack of such rebasing (i.e. porting a correction from scripted one).
Re: [ANNOUNCE] Git v2.20.0-rc1
Unless I hear otherwise in the next 24 hours, I am planning to merge the following topics to 'master' before cutting -rc2. Please stop me on any of these topics. - jc/postpone-rebase-in-c This may be the most controversial. It demotes the C reimplementation of "git rebase" to an experimental opt-in feature that can only be enabled by setting rebase.useBuiltIn configuration that defaults to false. cf. - ab/format-patch-rangediff-not-stat The "--rangediff" option recently added to "format-patch" interspersed a bogus and useless "--stat" information by mistake, which is being corrected. cf. The following three topics I do not need help in deciding; they are all good and will be merged before -rc2. - jk/t5562-perl-path-fix This is to invoke a perl scriptlet with "$PERL_PATH" in one of the new tests, instead of relying on (an incorrect) she-bang line in the script file. - tb/clone-case-smashing-warning-test This enables the test to see the behaviour of "git clone" after cloning a project that has paths that are different only in case on MINGW (earlier it wanted CASE_INSENSITIVE_FS prerequisite and in addition not on MINGW). - nd/per-worktree-ref-iteration This fixes a "return function_that_returns_void(...)" in a function that returns void. Thanks.
Re: [PATCH] doc: update diff-format.txt for removed ellipses in --raw
Greg Hurrell writes:
> Since 7cb6ac1e4b ("diff: diff_aligned_abbrev: remove ellipsis after
> abbreviated SHA-1 value", 2017-12-03), the "--raw" format of diff
> does not add ellipses in an attempt to align the output, but the
> documentation was not updated to reflect this.
>
> Signed-off-by: Greg Hurrell
> ---
>
> The GIT_PRINT_SHA1_ELLIPSIS environment variable can be used, for now,
> to bring back the old output format, but that is already documented in
> git.txt, so I am not mentioning it here.
Thanks. Will queue.
Re: [PATCH v3 2/5] pretty: allow showing specific trailers
Junio C Hamano writes:
> I was confused by the "only" stuff.
>
> When you give a key (or two), they cannot possibly name non-trailer
> lines, so while it may be possible to ask "oh, by the way, I also
> want non-trailer lines in addition to signed-off-by and cc lines",
> the value of being able to make such a request is dubious.
>
> The value is dubious, but logically it makes it more consistent with
> the current %(trailers) that lack 'only', which is "oh by the way, I
> also want non-trailer lines in addition to the trailers with
> keyword", to allow a way to countermand the 'only' you flip on by
> default when keywords are given.
Would it feel less inconsistent if it did not set the 'only_trailers'
option?
Now that I look at it again setting 'only_trailers' is more of an
implementation trick/hack to make sure it doesn't take the fast-path in
format_trailer_info (and by documenting it it justifies that hack). If
instead the 'filter' option is checked in the relevant places there
would be no need to mix up 'only' with 'filter'.
That is, do you think something like this should be squashed in?
diff --git a/pretty.c b/pretty.c
index 302e67fa8..f45ccaf51 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1360,7 +1360,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
opts.filter = format_trailer_match_cb;
opts.filter_data = _list;
- opts.only_trailers = 1;
} else
break;
}
diff --git a/trailer.c b/trailer.c
index 97c8f2762..07ca2b2c6 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,
size_t i;
/* If we want the whole block untouched, we can take the fast path. */
- if (!opts->only_trailers && !opts->unfold) {
+ if (!opts->only_trailers && !opts->unfold && !opts->filter) {
strbuf_add(out, info->trailer_start,
info->trailer_end - info->trailer_start);
return;
@@ -1156,7 +1156,7 @@ static void format_trailer_info(struct strbuf *out,
strbuf_release();
strbuf_release();
- } else if (!opts->only_trailers) {
+ } else if (!opts->only_trailers && !opts->filter) {
strbuf_addstr(out, trailer);
}
}
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 7548e1d38..ea3cd5b28 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -228,9 +228,9 @@ endif::git-rev-list[]
** 'key=': only show trailers with specified key. Matching is done
case-insensitively and trailing colon is optional. If option is
given multiple times trailer lines matching any of the keys are
- shown. Non-trailer lines in the trailer block are also hidden
- (i.e. 'key' implies 'only'). E.g., `%(trailers:key=Reviewed-by)`
- shows trailer lines with key `Reviewed-by`.
+ shown. Non-trailer lines in the trailer block are also hidden.
+ E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key
+ `Reviewed-by`.
** 'only': omit non-trailer lines from the trailer block.
** 'unfold': make it behave as if interpret-trailer's `--unfold`
option was given. E.g., `%(trailers:only,unfold)` unfolds and
Re: [RFC PATCH 7/7] config.mak.uname: use pkgsrc perl for NetBSD
Carlo Marcelo Arenas Belón writes: > otherwise will default to /usr/bin/perl which wouldn't normally exist > > Signed-off-by: Carlo Marcelo Arenas Belón > --- > config.mak.uname | 1 + > 1 file changed, 1 insertion(+) I do not have experience with NetBSD so I take your words on face value. This patch makes sense to me. Thanks. > diff --git a/config.mak.uname b/config.mak.uname > index 59ce03819b..d2edb723f4 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -249,6 +249,7 @@ ifeq ($(uname_S),NetBSD) > OLD_ICONV = UnfortunatelyYes > BASIC_CFLAGS += -I/usr/pkg/include > BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib > + PERL_PATH = /usr/pkg/bin/perl > USE_ST_TIMESPEC = YesPlease > HAVE_PATHS_H = YesPlease > HAVE_BSD_SYSCTL = YesPlease
Re: [RFC PATCH 6/7] t5004: use GNU tar to avoid known issues with BSD tar
Carlo Marcelo Arenas Belón writes:
> 56ee96572a ("t5004: resurrect original empty tar archive test", 2013-05-09)
> added a test to try to detect and workaround issues with the standard tar
> from BSD, but at least in NetBSD would be better to instead require GNU tar
> which is available from pkgsrc
>
> Signed-off-by: Carlo Marcelo Arenas Belón
> ---
> t/t5004-archive-corner-cases.sh | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
> index ced44355ca..baafc553f8 100755
> --- a/t/t5004-archive-corner-cases.sh
> +++ b/t/t5004-archive-corner-cases.sh
> @@ -31,6 +31,8 @@ test_lazy_prereq UNZIP_ZIP64_SUPPORT '
> "$GIT_UNZIP" -v | grep ZIP64_SUPPORT
> '
>
> +test $uname_s = NetBSD && TAR="gtar"
> +
This smells wrong.
Isn't the top-level Makefile ask you to use TAR=gtar if that is the
usable implementation of tar on your platform, and isn't what you
specify as $(TAR) exported down to the t/Makefile to be used here?
> # bsdtar/libarchive versions before 3.1.3 consider a tar file with a
> # global pax header that is not followed by a file record as corrupt.
> if "$TAR" tf "$TEST_DIRECTORY"/t5004/empty-with-pax-header.tar >/dev/null
> 2>&1
Re: [RFC PATCH 5/7] test-lib: use pkgsrc provided unzip for NetBSD
Carlo Marcelo Arenas Belón writes:
> d98b2c5fce ("test-lib: on FreeBSD, look for unzip(1) in /usr/local/bin/",
> 2016-07-21) added an exception to the test suite for FreeBSD because the
> tests assume functionality not provided by its base unzip tool.
>
> NetBSD shares that limitation and provides a package that could be used
> instead so all tests from t5003 succeed
>
> Signed-off-by: Carlo Marcelo Arenas Belón
> ---
> t/test-lib.sh | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 6c6c0af7a1..2acb35f277 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1244,6 +1244,7 @@ test_lazy_prereq SANITY '
> '
>
> test FreeBSD != $uname_s || GIT_UNZIP=${GIT_UNZIP:-/usr/local/bin/unzip}
> +test $uname_s = NetBSD && GIT_UNZIP=${GIT_UNZIP:-/usr/pkg/bin/unzip}
This is OK for now, but I wonder why this is done in test-lib.sh in
the first place, unlike $(TAR) that is set and configurable from the
top level. The difference is that GIT_UNZIP happens to be used only
in tests, while TAR is used in the primary build procedure.
But I suspect that in the longer run, we should allow UNZIP to be
given next to TAR to the top-level Makefile and export it down.
That would allow the usual mechanism like config.mak.uname,
./configure and "make TAR=... UNZIP=..." command line override
to be used uniformly, without people having to worry about the
distinction. The builders should *not* have to care that one is
used in the build and the other is only used in the test.
> GIT_UNZIP=${GIT_UNZIP:-unzip}
> test_lazy_prereq UNZIP '
> "$GIT_UNZIP" -v
Re: [RFC PATCH 4/7] config.mak.uname: NetBSD uses old iconv interface
Carlo Marcelo Arenas Belón writes: > prevents the following warning : s/^/Doing so / or something to make it a complete sentence. > ... > it is set by optional configure at least since NetBSD 6.0 s/it/It/; Again, makes sense, and thanks for tying this loose end. > Signed-off-by: Carlo Marcelo Arenas Belón > --- > config.mak.uname | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/config.mak.uname b/config.mak.uname > index 36c973c7e6..59ce03819b 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -246,6 +246,7 @@ ifeq ($(uname_S),NetBSD) > ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2) > NEEDS_LIBICONV = YesPlease > endif > + OLD_ICONV = UnfortunatelyYes > BASIC_CFLAGS += -I/usr/pkg/include > BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib > USE_ST_TIMESPEC = YesPlease
Re: [RFC PATCH 3/7] config.mak.uname: NetBSD uses BSD semantics with fread for directories
Carlo Marcelo Arenas Belón writes: > this "fixes" test 23 (proper error on directory "files") from t1308 > > other BSD (OpenBSD, MirBSD) likely also affected but they will be > fixed in a different series > > the optional 'configure' sets this automatically and is probably what > most users from this platform had been doing as a workaround Yup, thanks for straightening this out. > > Signed-off-by: Carlo Marcelo Arenas Belón > --- > config.mak.uname | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/config.mak.uname b/config.mak.uname > index 3ee7da0e23..36c973c7e6 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -253,6 +253,7 @@ ifeq ($(uname_S),NetBSD) > HAVE_BSD_SYSCTL = YesPlease > HAVE_BSD_KERN_PROC_SYSCTL = YesPlease > PROCFS_EXECUTABLE_PATH = /proc/curproc/exe > + FREAD_READS_DIRECTORIES = UnfortunatelyYes > endif > ifeq ($(uname_S),AIX) > DEFAULT_PAGER = more
Re: [RFC PATCH 2/7] t0301: remove trailing / for dir creation
Carlo Marcelo Arenas Belón writes: > the semantics of how mkdir -p should work, specially when using -m are > not standard and in this case NetBSD will assume that the permision > should not be changed, breaking the test This does not explain, except for the fuzzy "in this case", why we want to lose trailing slash at all. If what you wanted to say was On NetBSD, "mkdir -p -m $bits path/to/dir/" ignores the permission bits when creating the directory component 'dir', but without the trailing slash at the end of "dir/", it works as expected. then that would be an understandable justification for the patch. If you meant to say something else, then I couldn't read it from what you wrote, so the log message needs updating to help future readers. > -p is technically not needed either, but will be cleared in a future > patch eventhough it could be considered an alternative fix I haven't seen the steps 3-7/7 yet, but if they remove "-p", then "but" would be a strange thing to say here. If they indeed do, then perhaps: The '-p' option is not needed in thse cases, as we know $HOME/ exists in the test environment and we are creating a new directory directly under it. It will be removed in a future patch in the series. Removing '-p' could be an alternative fix, as the command then works as expected even on NetBSD with the trailing slash after directory name. On the other hand, if future changes make it necessary to create two or more levels here, then At this point in the series, '-p' is not needed in thse cases, as we know $HOME/ exists in the test environment and we are creating .git-credential-cache directory directly under it. However, in later patches, we'll make it necessary for these 'mkdir -p' to create two or more levels, so removing '-p' would be an alternative fix for this step, but would not work for the series as a whole. would also make sense. I simply do not have enough information yet to tell which at this point in the series. > Signed-off-by: Carlo Marcelo Arenas Belón > --- > t/t0301-credential-cache.sh | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh > index fd92533acf..9529c612af 100755 > --- a/t/t0301-credential-cache.sh > +++ b/t/t0301-credential-cache.sh > @@ -77,9 +77,9 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg > socket exists" ' > test_expect_success 'use user socket if user directory exists' ' > test_when_finished " > git credential-cache exit && > - rmdir \"\$HOME/.git-credential-cache/\" > + rmdir \"\$HOME/.git-credential-cache\" > " && > - mkdir -p -m 700 "$HOME/.git-credential-cache/" && > + mkdir -p -m 700 "$HOME/.git-credential-cache" && > check approve cache <<-\EOF && > protocol=https > host=example.com > @@ -92,10 +92,10 @@ test_expect_success 'use user socket if user directory > exists' ' > test_expect_success SYMLINKS 'use user socket if user directory is a symlink > to a directory' ' > test_when_finished " > git credential-cache exit && > - rmdir \"\$HOME/dir/\" && > + rmdir \"\$HOME/dir\" && > rm \"\$HOME/.git-credential-cache\" > " && > - mkdir -p -m 700 "$HOME/dir/" && > + mkdir -p -m 700 "$HOME/dir" && > ln -s "$HOME/dir" "$HOME/.git-credential-cache" && > check approve cache <<-\EOF && > protocol=https
Re: [RFC PATCH 1/7] Documentation: update INSTALL for NetBSD
Carlo Marcelo Arenas Belón writes: > NetBSD added a BSD licensed reimplementation of GNU libintl to > its base at least since release 4.0 (mid 2012) and git can be > configured to build with it. > > Signed-off-by: Carlo Marcelo Arenas Belón > --- > INSTALL | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Looks good. > diff --git a/INSTALL b/INSTALL > index c39006e8e7..100718989f 100644 > --- a/INSTALL > +++ b/INSTALL > @@ -154,11 +154,11 @@ Issues of note: > git-gui, you can use NO_TCLTK. > > - A gettext library is used by default for localizing Git. The > - primary target is GNU libintl, but the Solaris gettext > - implementation also works. > + primary target is GNU libintl, but the Solaris and NetBSD gettext > + implementations also work. > > We need a gettext.h on the system for C code, gettext.sh (or > - Solaris gettext(1)) for shell scripts, and libintl-perl for Perl > + gettext(1) utility) for shell scripts, and libintl-perl for Perl > programs. > > Set NO_GETTEXT to disable localization support and make Git only
[PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
Junio C Hamano writes:
> Ævar Arnfjörð Bjarmason writes:
>
>>> * "git rebase" and "git rebase -i" have been reimplemented in C.
>>
>> Here's another regression in the C version (and rc1),...
>> I wasn't trying to stress test rebase. I was just wanting to rebase a
>> history I was about to force-push after cleaning it up, hardly an
>> obscure use-case. So [repeat last transmission in
>> https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/ ]
>
> which, to those who are reading from sidelines:
>
> Given that we're still finding regressions bugs in the rebase-in-C
> version should we be considering reverting 5541bd5b8f ("rebase: default
> to using the builtin rebase", 2018-08-08)?
>
> I love the feature, but fear that the current list of known regressions
> serve as a canary for a larger list which we'd discover if we held off
> for another major release (and would re-enable rebase.useBuiltin=true in
> master right after 2.20 is out the door).
>
> I am fine with the proposed flip, but I'll have to see the extent of
> damage this late in the game so that I won't miss anything. In
> addition to the one-liner below, we'd need to update the quoted
> release notes entry, and possibly adjust some tests (even though the
> "reimplementation" ought to be bug-to-bug compatible, it may not be).
So, in a more concrete form, what you want to see is something like
this in -rc2 and later?
-- >8 --
Subject: [PATCH] rebase: mark the C reimplementation as an experimental opt-in
feature
It turns out to be a bit too early to unleash the reimplementation
to the general public. Let's rewrite some documentation and make it
an opt-in feature.
Signed-off-by: Junio C Hamano
---
Documentation/config/rebase.txt | 16 ++--
builtin/rebase.c| 2 +-
t/README| 4 ++--
3 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index f079bf6b7e..af12623151 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -1,16 +1,12 @@
rebase.useBuiltin::
- Set to `false` to use the legacy shellscript implementation of
- linkgit:git-rebase[1]. Is `true` by default, which means use
- the built-in rewrite of it in C.
+ Set to `true` to use the experimental reimplementation of
+ linkgit:git-rebase[1] in C. Defaults to `false`.
+
The C rewrite is first included with Git version 2.20. This option
-serves an an escape hatch to re-enable the legacy version in case any
-bugs are found in the rewrite. This option and the shellscript version
-of linkgit:git-rebase[1] will be removed in some future release.
-+
-If you find some reason to set this option to `false` other than
-one-off testing you should report the behavior difference as a bug in
-git.
+allows early adopters to opt into the experimental version to find
+bugs in the rewritten version. This option and the shellscript version
+of linkgit:git-rebase[1] will be removed in some future release once
+the reimplementation becomes more stable.
rebase.stat::
Whether to show a diffstat of what changed upstream since the last
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8..19ad97b177 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -59,7 +59,7 @@ static int use_builtin_rebase(void)
cp.git_cmd = 1;
if (capture_command(, , 6)) {
strbuf_release();
- return 1;
+ return 0;
}
strbuf_trim();
diff --git a/t/README b/t/README
index 28711cc508..7e925e5fea 100644
--- a/t/README
+++ b/t/README
@@ -345,8 +345,8 @@ for the index version specified. Can be set to any valid
version
GIT_TEST_PRELOAD_INDEX= exercises the preload-index code path
by overriding the minimum number of cache entries required per thread.
-GIT_TEST_REBASE_USE_BUILTIN=, when false, disables the
-builtin version of git-rebase. See 'rebase.useBuiltin' in
+GIT_TEST_REBASE_USE_BUILTIN=, when true, forces the use of
+builtin version of git-rebase in the test. See 'rebase.useBuiltin' in
git-config(1).
GIT_TEST_INDEX_THREADS= enables exercising the multi-threaded loading
--
2.20.0-rc1
Re: [PATCH v11 00/22] Convert "git stash" to C builtin
Thomas Gummerer writes: > Thanks for your work on this! I have read through the range-diff and > the new patch of this last round, and this addresses all the comments > I had on v10 (and some more :)). I consider it > Reviewed-by: Thomas Gummerer Thanks. One thing that bothers me is that this seems to have been rebased on 'master', but as long as we are rebasing, the updated series must also take into account of the sd/stash-wo-user-name topic, i.e. if we are rebasing it, it should be rebased on top of the result of git checkout -B ps/rebase-in-c master git merge --no-ff sd/stash-wo-user-name I think.
Re: [PATCH v11 20/22] stash: convert `stash--helper.c` into `stash.c`
Paul-Sebastian Ungureanu writes:
> The old shell script `git-stash.sh` was removed and replaced
> entirely by `builtin/stash.c`. In order to do that, `create` and
> `push` were adapted to work without `stash.sh`. For example, before
> this commit, `git stash create` called `git stash--helper create
> --message "$*"`. If it called `git stash--helper create "$@"`, then
> some of these changes wouldn't have been necessary.
>
> This commit also removes the word `helper` since now stash is
> called directly and not by a shell script.
>
> Signed-off-by: Paul-Sebastian Ungureanu
> ---
> .gitignore | 1 -
> Makefile | 3 +-
> builtin.h| 2 +-
> builtin/{stash--helper.c => stash.c} | 157 +++
> git-stash.sh | 153 --
> git.c| 2 +-
> 6 files changed, 92 insertions(+), 226 deletions(-)
> rename builtin/{stash--helper.c => stash.c} (91%)
> delete mode 100755 git-stash.sh
Seeing the recent trouble in "rebase in C" and how keeping the
scripted version as "git legacy-rebase" helped us postpone the
rewritten version without ripping the whole thing out, I wonder if
we can do the same here.
Also, the remaining two patches should be done _before_ this step, I
would think. I can understand it if the reason you have those two
after this step is because you found the opportunity for these
improvements after you wrote this step, but in the larger picture
seen by the end users of the "stash in C" and those developers who
follow the evolution of the code, the logical place for this "now we
have everything in C, we retire the scripted version" step to happen
is at the very end.
Thanks.
Re: [PATCH] files-backend.c: fix build error on Solaris
Carlo Arenas writes: > Signed-off-by: Carlo Marcelo Arenas Belón Do you mean Tested-by: (meaning, you actually saw the breakage with SunCC without the patch and also saw the patch fixed the breakage)? > clang with -Wpedantic also catch this (at least with Apple LLVM > version 10.0.0); recent versions of gcc also include that flag and at > least 8.2.0 shows a warning for it, so it might be worth adding it to > developer mode (maybe under the pedantic DEVOPTS). Nice to know. > ..., would this be something I should be adding?, what are the expectations > around > warnings and compilers? It may be a worthwhile thing to discuss, and as a discussion starter, a patch would help ;-).
Re: [PATCH] setup.c: remove needless argument passed to open in sanitize_stdfds
pedrodel...@gmail.com writes: > According to POSIX manual pages, the open() system call's mode > argument specifies the file mode bits to be applied when a new > file is created. If neither O_CREAT nor O_TMPFILE is specified, > then mode is ignored. Correct. While I would say two argument form would have been better if this were a new code, this change is borderline Meh for existing code, because passing 0 there already gives a reasonable sign that we know this parameter does not matter. If the ignored parameter were 0666 or some other more plausible-as-perm-bits value, the story would be quite different, though. Thanks.
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Johannes Sixt writes: > But incorrect whitespace is never highlighted in removed lines, why > should CR be an exception? > ... > Same here for other cases, for example > > -something > +something > > will not have on obvious indicator that whitespace was corrected. All correct, but misses one point in Frank's original report, which observed -something +something_new^M with ^M highlighted for whitespace error. The highlighting is correct. But notice lack of caret-em on the preimage line? It turns out that we show something like this -something CR LF for the preimage line, while showing something like this +something_new CR LF for the postimage line. Because CR on the postimage line, thanks to highlighting, appears alone separate from the LF, it is shown as two-letter caret-em sequence to the user. On the other hand, because CR and LF appear next to each other on the preimage line, the pager and/or the terminal behaves as if CR is not even there and that is where Frank's complaint comes from, I think. The code is doing the right thing by showing CR, but it is hidden by the pager and/or the terminal.
Re: [PATCH v3 2/5] pretty: allow showing specific trailers
Anders Waldenborg writes: > Junio C Hamano writes: >> Also, use of 'key=' automatically turns on 'only' as described, and >> I tend to agree that it would a convenient default mode (i.e. when >> picking certain trailers only with this mechanism, it is likely that >> the user is willing to use %(subject) etc. to fill in what was lost >> by the implicit use of 'only'), but at the same time, it makes me >> wonder if we need to have a way to countermand an 'only' (or >> 'unfold' for that matter) that was given earlier, e.g. >> >> %(trailers:key=signed-off-by,only=no) >> >> Thanks. > > I'm not sure what that would mean. The non-trailer lines in the trailer > block doesn't match the key. I was confused by the "only" stuff. When you give a key (or two), they cannot possibly name non-trailer lines, so while it may be possible to ask "oh, by the way, I also want non-trailer lines in addition to signed-off-by and cc lines", the value of being able to make such a request is dubious. The value is dubious, but logically it makes it more consistent with the current %(trailers) that lack 'only', which is "oh by the way, I also want non-trailer lines in addition to the trailers with keyword", to allow a way to countermand the 'only' you flip on by default when keywords are given.
Re: [RFC] Introduce two new commands, switch-branch and restore-paths
Thomas Gummerer writes: > I like the idea of splitting those commands up, in fact it is > something I've been considering working on myself. I do think we > should consider if we want to change the behaviour of those new > commands in any way compared to 'git checkout', since we're starting > with a clean slate. > > One thing in particular that I have in mind is something I'm currently > working on, namely adding a --index flag to 'git checkout', which > would make 'git checkout' work in non-overlay mode (for more > discussion on that see also [*1*]. Ah, thanks for reminding me of that. That explains why I felt uneasy to see "restore" in the proposed command name. In short, I think "checkout --index ", i.e. if the matches a directory in and the current index and/or the working tree has tracked paths in that directory that do not exist in , the operation _removes_ these paths so that the result matches , should become the default of "I want to check out these paths from the named tree-ish". The current one is not exactly "checking out the paths" in that it ignores and does not check out the absense of paths in . That operation sounds more like "restoring paths out of a given tree". If the tree does not have some paths, these paths won't be "restored" from that tree, so "restore" matches the current "overlay what's taken out of the given tree on top of what is already in the index and the working tree, without checking out the absense of paths" better from that point of view.
Re: [PATCH] http-backend: enable cleaning up forked upload/receive-pack on exit
Max Kirillov writes: > If http-backend dies because of errors, started upload-pack or > receive-pack are not killed and waited, but rather stay running for somtime "sometime" (will fix locally, no reason for a resend). > until they exits because of closed stdin. It may be undesirable in working "they exit" (ditto) > environment, and it also causes occasional failure of t5562, because the > processes keep opened act.err, and sometimes write there errors after next > test > started using the file. > > Fix by enabling cleaning of the command at http-backed exit. Thanks for a clear explanation. Will queue. > Reported-by: Carlo Arenas > Helped-by: Carlo Arenas > Signed-off-by: Max Kirillov > --- > This seems to fix the issue at NetBSD. I verified it manually with strace but > could > not catch the visible timing effect in tests at Linux. So no tests for it. > > the "t5562: do not reuse output files" patches are not needed then > http-backend.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/http-backend.c b/http-backend.c > index 9e894f197f..29e68e38b5 100644 > --- a/http-backend.c > +++ b/http-backend.c > @@ -486,6 +486,8 @@ static void run_service(const char **argv, int > buffer_input) > if (buffer_input || gzipped_request || req_len >= 0) > cld.in = -1; > cld.git_cmd = 1; > + cld.clean_on_exit = 1; > + cld.wait_after_clean = 1; > if (start_command()) > exit(1);
Re: [PATCH] t5562: do not reuse output files
Max Kirillov writes:
> On Sat, Nov 24, 2018 at 04:47:19PM +0900, Junio C Hamano wrote:
>> I do agree that forcing the parent to wait, like you described in
>> the comment, would be far more preferrable,
>
> It looks like it can be done as simple as:
>
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -486,6 +486,8 @@ static void run_service(const char **argv, int
> buffer_input)
> if (buffer_input || gzipped_request || req_len >= 0)
> cld.in = -1;
> cld.git_cmd = 1;
> + cld.clean_on_exit = 1;
> + cld.wait_after_clean = 1;
> if (start_command())
> exit(1);
>
> at least according to strate it does what it should.
Sounds sane.
I am offhand not sure what the right value of wait_after_clean for
this codepath be, though. 46df6906 ("execv_dashed_external: wait
for child on signal death", 2017-01-06) made this non-default but
turned it on for dashed externals (especially to help the case where
they spawn a pager), as the parent has nothing other than to wait
for the child to exit in that codepath. Does the same reasoning
apply here, too?
This is a meta point, but I wonder if there is an easy way to "grep"
for uses of run-command interface that do *not* set clean_on_exit.
The pager that can outlive us long after we exit, kept alive while
the user views our output, is an example cited by afe19ff7
("run-command: optionally kill children on exit", 2012-01-07), and
while I am wondering if the default should hae been to kill the
children instead, such a "grep" would have been very useful to know
what codepaths would be affected if we flipped the default.
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
On Sun, Nov 25, 2018 at 03:03:11PM +0100, Frank Schäfer wrote: > Am 24.11.18 um 23:07 schrieb Johannes Sixt: > > I don't think that there is anything to fix. If you have a file with > > CRLF in it, but you did not declare to Git that CRLF is the expected > > end-of-line indicator, then the CR *is* trailing whitespace (because > > the line ends at LF), and 'git diff' highlights it. > > Sure, it's correct to highlight it. > But it doesn't highlight it in removed lines, just in added lines. > I can see no good reason why removed and added lines should be treated > differently. The default behavior is to highlight whitespace errors only in new lines, because the assumption is that while you don't want to introduce new errors, you can't do anything about old mistakes without rewriting history. I agree that in many circumstances, such as code review, this may be undesirable. In the past, I've done code reviews where I may let existing trailing whitespace go but am strict about not introducing new trailing whitespace, and being able to see both is helpful. If you want to see whitespace errors in both the old and the new, use --ws-error-highlight or set diff.wsErrorHighlight. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH v3 2/5] pretty: allow showing specific trailers
Junio C Hamano writes: > Also, use of 'key=' automatically turns on 'only' as described, and > I tend to agree that it would a convenient default mode (i.e. when > picking certain trailers only with this mechanism, it is likely that > the user is willing to use %(subject) etc. to fill in what was lost > by the implicit use of 'only'), but at the same time, it makes me > wonder if we need to have a way to countermand an 'only' (or > 'unfold' for that matter) that was given earlier, e.g. > > %(trailers:key=signed-off-by,only=no) > > Thanks. I'm not sure what that would mean. The non-trailer lines in the trailer block doesn't match the key. Take this commit as an example: $ git show -s --pretty=format:'%(trailers)' b4d065df03049bacfbc40467b60b13e804b7d289 Helped-by: Jeff King [jc: took idea and log message from peff] Signed-off-by: Junio C Hamano With 'only' it shows: $ git show -s --pretty=format:'%(trailers:only)' b4d065df03049bacfbc40467b60b13e804b7d289 Helped-by: Jeff King Signed-off-by: Junio C Hamano Now with a "key=signed-off-by" option I would imagine that as adding a "| grep -i '^signed-off-by:'" to the end. In both cases (with and without 'only') that would give the same result: "Signed-off-by: Junio C Hamano " anders
Re: t5570 shaky for anyone ?
On Sun, Nov 25, 2018 at 09:52:23PM +0100, Torsten Bögershausen wrote: > After running the "Git 2.20-rc1" testsuite here on a raspi, > the only TC that failed was t5570. > When the "grep" was run on daemon.log, the file was empty (?). > When inspecting it later, it was filled, and grep would have found > the "extended.attribute" it was looking for. I think I saw the same failure on Travis CI already before 2.19, so it's not a new issue. Here's the test's verbose output: + cat + + GIT_OVERRIDE_VIRTUAL_HOST=localhost git -c protocol.version=1 ls-remote git://127.0.0.1:5570/interp.git b6752e52dd867264d12240028003f21e3e1dccabHEAD b6752e52dd867264d12240028003f21e3e1dccabrefs/heads/master + cut -d -f2- + grep -i extended.attribute daemon.log + test_cmp expect actual + diff -u expect actual --- expect 2018-06-12 10:06:50.758357927 + +++ actual 2018-06-12 10:06:50.774365936 + @@ -1,2 +0,0 @@ -Extended attribute "host": localhost -Extended attribute "protocol": version=1 [10579] Connection from 127.0.0.1:45836 [10579] Extended attribute "host": localhost [10579] Extended attribute "protocol": version=1 error: last command exited with $?=1 [10579] Request upload-pack for '/interp.git' [10579] Interpolated dir '/usr/src/git/t/trash dir.t5570/repo/localhost/interp.git' [10462] [10579] Disconnected not ok 21 - daemon log records all attributes The thing is that 'git daemon's log is not written to 'daemon.log' directly, but it goes through a fifo, which is read by a shell loop, which then sends all log messages both to 'daemon.log' and to the test script's standard error. So there is certainly a race between log messages going through the fifo and the loop before reaching 'daemon.log' and 'git ls-remote' exiting and 'grep' opening 'daemon.log'. > The following fixes it, but I am not sure if this is the ideal > solution. Currently this is the only test that looks at 'daemon.log', but if we ever going to add another one, then that will be prone to the same issue. I wonder whether it's really that useful to have the daemon log in the test script's output... if the log was sent directly to daemon log, then the window for this race would be smaller, but still not completely closed. Anyway, I added Peff to Cc, since he added that whole fifo-shell-loop thing. > diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh > index 7466aad111..e259fee0ed 100755 > --- a/t/t5570-git-daemon.sh > +++ b/t/t5570-git-daemon.sh > @@ -192,6 +192,7 @@ test_expect_success 'daemon log records all attributes' ' > GIT_OVERRIDE_VIRTUAL_HOST=localhost \ > git -c protocol.version=1 \ > ls-remote "$GIT_DAEMON_URL/interp.git" && > + sleep 1 && > grep -i extended.attribute daemon.log | cut -d" " -f2- >actual && > test_cmp expect actual > ' > > A slightly better approach may be to use a "sleep on demand": > > + ( grep -i -q extended.attribute daemon.log || sleep 1 ) && >
Re: [RFC] Introduce two new commands, switch-branch and restore-paths
On 11/20, Duy Nguyen wrote:
> On Mon, Nov 19, 2018 at 04:19:53PM +0100, Duy Nguyen wrote:
> > I promise to come back with something better (at least it still
> > sounds better in my mind). If that idea does not work out, we can
> > come back and see if we can improve this.
>
> So this is it. The patch isn't pretty, mostly as a proof of
> concept. Just look at the three functions at the bottom of checkout.c,
> which is the main thing.
>
> This patch tries to split "git checkout" command in two new ones:
>
> - git switch-branch is all about switching branches
> - git restore-paths (maybe restore-file is better) for checking out
> paths
>
> The main idea is these two commands will co-exist with the good old
> 'git checkout', which will NOT be deprecated. Old timers will still
> use "git checkout". But new people should be introduced to the new two
> instead. And the new ones are just as capable as "git checkout".
>
> Since the three commands will co-exist (with duplicate functionality),
> maintenance cost must be kept to minimum. The way I did this is simply
> split the command line options into three pieces: common,
> switch-branch and checkout-paths. "git checkout" has all three, the
> other two have common and another piece.
>
> With this, a new option added to git checkout will be automatically
> available in either switch-branch or checkout-paths. Bug fixes apply
> to all relevant commands.
>
> Later on, we could start to add a bit more stuff in, e.g. some form of
> disambiguation is no longer needed when running as switch-branch, or
> restore-paths.
>
> So, what do you think?
I like the idea of splitting those commands up, in fact it is
something I've been considering working on myself. I do think we
should consider if we want to change the behaviour of those new
commands in any way compared to 'git checkout', since we're starting
with a clean slate.
One thing in particular that I have in mind is something I'm currently
working on, namely adding a --index flag to 'git checkout', which
would make 'git checkout' work in non-overlay mode (for more
discussion on that see also [*1*]. I got something working, that
needs to be polished a bit and am hoping to send that to the list
sometime soon.
I wonder if such the --index behaviour could be the default in
restore-paths command?
Most of the underlying machinery for 'checkout' could and should of
course still be shared between the commands.
*1*:
> -- 8< --
> diff --git a/builtin.h b/builtin.h
> index 6538932e99..6e321ec8a4 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -214,6 +214,7 @@ extern int cmd_remote_fd(int argc, const char **argv,
> const char *prefix);
> extern int cmd_repack(int argc, const char **argv, const char *prefix);
> extern int cmd_rerere(int argc, const char **argv, const char *prefix);
> extern int cmd_reset(int argc, const char **argv, const char *prefix);
> +extern int cmd_restore_paths(int argc, const char **argv, const char
> *prefix);
> extern int cmd_rev_list(int argc, const char **argv, const char *prefix);
> extern int cmd_rev_parse(int argc, const char **argv, const char *prefix);
> extern int cmd_revert(int argc, const char **argv, const char *prefix);
> @@ -227,6 +228,7 @@ extern int cmd_show_index(int argc, const char **argv,
> const char *prefix);
> extern int cmd_status(int argc, const char **argv, const char *prefix);
> extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
> extern int cmd_submodule__helper(int argc, const char **argv, const char
> *prefix);
> +extern int cmd_switch_branch(int argc, const char **argv, const char
> *prefix);
> extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
> extern int cmd_tag(int argc, const char **argv, const char *prefix);
> extern int cmd_tar_tree(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index acdafc6e4c..868ca3c223 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -33,6 +33,16 @@ static const char * const checkout_usage[] = {
> NULL,
> };
>
> +static const char * const switch_branch_usage[] = {
> + N_("git switch-branch [] "),
> + NULL,
> +};
> +
> +static const char * const restore_paths_usage[] = {
> + N_("git restore-paths [] [] -- ..."),
> + NULL,
> +};
> +
> struct checkout_opts {
> int patch_mode;
> int quiet;
> @@ -44,6 +54,7 @@ struct checkout_opts {
> int ignore_skipworktree;
> int ignore_other_worktrees;
> int show_progress;
> + int dwim_new_local_branch;
> /*
>* If new checkout options are added, skip_merge_working_tree
>* should be updated accordingly.
> @@ -55,6 +66,7 @@ struct checkout_opts {
> int new_branch_log;
> enum branch_track track;
> struct diff_options diff_options;
> + char *conflict_style;
>
> int branch_exists;
> const char *prefix;
> @@ -1223,78 +1235,105 @@ static int
Re: t5570 shaky for anyone ?
On 11/25, Torsten Bögershausen wrote: > After running the "Git 2.20-rc1" testsuite here on a raspi, > the only TC that failed was t5570. > When the "grep" was run on daemon.log, the file was empty (?). > When inspecting it later, it was filled, and grep would have found > the "extended.attribute" it was looking for. I believe this has been reported before in https://public-inbox.org/git/1522783990.964448.1325338528.0d49c...@webmail.messagingengine.com/, but it seems like the thread never ended with actually fixing it. Reading the first reply Peff seemed to be fine with just removing the test completely, which would be the easiest solution ;) Adding him to Cc: here. > The following fixes it, but I am not sure if this is the ideal > solution. > > > diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh > index 7466aad111..e259fee0ed 100755 > --- a/t/t5570-git-daemon.sh > +++ b/t/t5570-git-daemon.sh > @@ -192,6 +192,7 @@ test_expect_success 'daemon log records all attributes' ' > GIT_OVERRIDE_VIRTUAL_HOST=localhost \ > git -c protocol.version=1 \ > ls-remote "$GIT_DAEMON_URL/interp.git" && > + sleep 1 && > grep -i extended.attribute daemon.log | cut -d" " -f2- >actual && > test_cmp expect actual > ' > > A slightly better approach may be to use a "sleep on demand": > > + ( grep -i -q extended.attribute daemon.log || sleep 1 ) && >
Re: [PATCH v11 00/22] Convert "git stash" to C builtin
On 11/23, Paul-Sebastian Ungureanu wrote: > Hello, > > This is the 11th iteration of C git stash. Here are some of the changes, > based on Thomas's and dscho's suggestions (from mailing list / pull request > #495): Thanks for your work on this! I have read through the range-diff and the new patch of this last round, and this addresses all the comments I had on v10 (and some more :)). I consider it Reviewed-by: Thomas Gummerer > - improved memory management. Now, the callers of `do_create_stash()` > are responsible of freeing the parameter they pass in. Moreover, the > stash message is now a pointer to a buffer (in the previous iteration > it was a pointer to a string). This should make it more clear who is > responsible of freeing the memory. > > - added `strbuf_insertf()` which inserts a format string at a given > position in the buffer. > > - some minor changes (changed "!oidcmp" to "oideq") > > - fixed merge conflicts > > Best regards, > Paul > > Joel Teichroeb (5): > stash: improve option parsing test coverage > stash: convert apply to builtin > stash: convert drop and clear to builtin > stash: convert branch to builtin > stash: convert pop to builtin > > Paul-Sebastian Ungureanu (17): > sha1-name.c: add `get_oidf()` which acts like `get_oid()` > strbuf.c: add `strbuf_join_argv()` > strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()` > t3903: modernize style > stash: rename test cases to be more descriptive > stash: add tests for `git stash show` config > stash: mention options in `show` synopsis > stash: convert list to builtin > stash: convert show to builtin > stash: convert store to builtin > stash: convert create to builtin > stash: convert push to builtin > stash: make push -q quiet > stash: convert save to builtin > stash: convert `stash--helper.c` into `stash.c` > stash: optimize `get_untracked_files()` and `check_changes()` > stash: replace all `write-tree` child processes with API calls > > Documentation/git-stash.txt |4 +- > Makefile |2 +- > builtin.h|1 + > builtin/stash.c | 1596 ++ > cache.h |1 + > git-stash.sh | 752 > git.c|1 + > sha1-name.c | 19 + > strbuf.c | 51 ++ > strbuf.h | 16 + > t/t3903-stash.sh | 192 ++-- > t/t3907-stash-show-config.sh | 83 ++ > 12 files changed, 1897 insertions(+), 821 deletions(-) > create mode 100644 builtin/stash.c > delete mode 100755 git-stash.sh > create mode 100755 t/t3907-stash-show-config.sh > > -- > 2.19.1.878.g0482332a22 >
Re: [PATCH v11 03/22] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`
On 11/23, Paul-Sebastian Ungureanu wrote:
> Implement `strbuf_insertf()` and `strbuf_vinsertf()` to
> insert data using a printf format string.
>
> Original-idea-by: Johannes Schindelin
> Signed-off-by: Paul-Sebastian Ungureanu
> ---
> strbuf.c | 36
> strbuf.h | 9 +
> 2 files changed, 45 insertions(+)
>
> diff --git a/strbuf.c b/strbuf.c
> index 82e90f1dfe..bfbbdadbf3 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -249,6 +249,42 @@ void strbuf_insert(struct strbuf *sb, size_t pos, const
> void *data, size_t len)
> strbuf_splice(sb, pos, 0, data, len);
> }
>
> +void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt, va_list
> ap)
> +{
> + int len, len2;
> + char save;
> + va_list cp;
> +
> + if (pos > sb->len)
> + die("`pos' is too far after the end of the buffer");
I was going to ask about translation of this and other messages in
'die()' calls, but I see other messages in strbuf.c are not marked for
translation either. It may make sense to mark them all for
translation at some point in the future, but having them all
untranslated for now makes sense.
In the long run it may even be better to return an error here rather
than 'die()'ing, but again this is consistent with the rest of the
API, so this wouldn't be a good time to take that on.
> + va_copy(cp, ap);
> + len = vsnprintf(sb->buf + sb->len, 0, fmt, cp);
Here we're just getting the length of what we're trying to format
(excluding the final NUL). As the second argument is 0, we do not
modify the strbuf at this point...
> + va_end(cp);
> + if (len < 0)
> + BUG("your vsnprintf is broken (returned %d)", len);
> + if (!len)
> + return; /* nothing to do */
> + if (unsigned_add_overflows(sb->len, len))
> + die("you want to use way too much memory");
> + strbuf_grow(sb, len);
... and then we grow the strbuf by the length we previously, which
excludes the NUL character, plus one extra character, so even if pos
== len we are sure to have enough space in the strbuf ...
> + memmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);
> + /* vsnprintf() will append a NUL, overwriting one of our characters */
> + save = sb->buf[pos + len];
> + len2 = vsnprintf(sb->buf + pos, sb->alloc - sb->len, fmt, ap);
... and we use vsnprintf to write the formatted string to the
beginning of the buffer. sb->alloc - sb->len can be larger than
'len', which is fine as vsnprintf doesn't write anything after the NUL
character. And as 'strbuf_grow' adds len + 1 bytes to the strbuf
we'll always have enough space for adding the formatted string ...
> + sb->buf[pos + len] = save;
> + if (len2 != len)
> + BUG("your vsnprintf is broken (returns inconsistent lengths)");
> + strbuf_setlen(sb, sb->len + len);
And finally we set the strbuf to the new length. So all this is just
a very roundabout way to say that this function does the right thing
according to my reading (and tests).
> +}
> +
> +void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...)
> +{
> + va_list ap;
> + va_start(ap, fmt);
> + strbuf_vinsertf(sb, pos, fmt, ap);
> + va_end(ap);
> +}
> +
> void strbuf_remove(struct strbuf *sb, size_t pos, size_t len)
> {
> strbuf_splice(sb, pos, len, "", 0);
> diff --git a/strbuf.h b/strbuf.h
> index be02150df3..8f8fe01e68 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -244,6 +244,15 @@ void strbuf_addchars(struct strbuf *sb, int c, size_t n);
> */
> void strbuf_insert(struct strbuf *sb, size_t pos, const void *, size_t);
>
> +/**
> + * Insert data to the given position of the buffer giving a printf format
> + * string. The contents will be shifted, not overwritten.
> + */
> +void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt,
> + va_list ap);
> +
> +void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...);
> +
> /**
> * Remove given amount of data from a given position of the buffer.
> */
> --
> 2.19.1.878.g0482332a22
>
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 25.11.18 um 15:03 schrieb Frank Schäfer: Am 24.11.18 um 23:07 schrieb Johannes Sixt: I don't think that there is anything to fix. If you have a file with CRLF in it, but you did not declare to Git that CRLF is the expected end-of-line indicator, then the CR *is* trailing whitespace (because the line ends at LF), and 'git diff' highlights it. Sure, it's correct to highlight it. But it doesn't highlight it in removed lines, just in added lines. I can see no good reason why removed and added lines should be treated differently. But incorrect whitespace is never highlighted in removed lines, why should CR be an exception? 1) If CR+LF line termination is used in a file, changing the content of a line (but not its termination) currently produces a diff like -something +something_new^M which causes the user to think he has changed the line ending (added a CR) although he didn't. But this is not limited to CR at EOL: -something +something_new will also show the incorrect whitespace highlighted only for the + line. 2) If someone/something unintentionally changes the line termination from CR+LF to LF, it doesn't show up in the diff: -something +something Same here for other cases, for example -something +something will not have on obvious indicator that whitespace was corrected. If you are worried about a change in EOL style, you should better listen to your other tools. Either it is important, or it is not. If it is, they will report it to you. If it isn't, why care? -- Hannes
t5570 shaky for anyone ?
After running the "Git 2.20-rc1" testsuite here on a raspi, the only TC that failed was t5570. When the "grep" was run on daemon.log, the file was empty (?). When inspecting it later, it was filled, and grep would have found the "extended.attribute" it was looking for. The following fixes it, but I am not sure if this is the ideal solution. diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh index 7466aad111..e259fee0ed 100755 --- a/t/t5570-git-daemon.sh +++ b/t/t5570-git-daemon.sh @@ -192,6 +192,7 @@ test_expect_success 'daemon log records all attributes' ' GIT_OVERRIDE_VIRTUAL_HOST=localhost \ git -c protocol.version=1 \ ls-remote "$GIT_DAEMON_URL/interp.git" && + sleep 1 && grep -i extended.attribute daemon.log | cut -d" " -f2- >actual && test_cmp expect actual ' A slightly better approach may be to use a "sleep on demand": + ( grep -i -q extended.attribute daemon.log || sleep 1 ) &&
[PATCH] setup.c: remove needless argument passed to open in sanitize_stdfds
From: Pedro de Lyra
Signed-off-by: Pedro de Lyra
---
According to POSIX manual pages, the open() system call's mode argument
specifies the file mode bits to be applied when a new file is created. If
neither O_CREAT nor O_TMPFILE is specified, then mode is ignored. So I guess
that 0 argument only confuses the reader.
setup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/setup.c b/setup.c
index 1be5037f1..d4fd14bb7 100644
--- a/setup.c
+++ b/setup.c
@@ -1228,7 +1228,7 @@ const char *resolve_gitdir_gently(const char *suspect,
int *return_error_code)
/* if any standard file descriptor is missing open it to /dev/null */
void sanitize_stdfds(void)
{
- int fd = open("/dev/null", O_RDWR, 0);
+ int fd = open("/dev/null", O_RDWR);
while (fd != -1 && fd < 2)
fd = dup(fd);
if (fd == -1)
--
2.17.1
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Sun, Nov 25 2018, Torsten Bögershausen wrote:
> On Sun, Nov 25, 2018 at 05:28:35AM +0100, Torsten Bögershausen wrote:
>> On Sat, Nov 24, 2018 at 08:33:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >
>> > On Wed, Sep 05 2018, Ævar Arnfjörð Bjarmason wrote:
>> >
>> > > On Wed, Sep 05 2018, Eric Sunshine wrote:
>>
>> []
>>
>> > > SunCC used to be ahead of GCC & Clang when it came to certain classes of
>> > > warnings, but e.g. now everything it complains about is because it
>> > > doesn't understand C as well, e.g. we have quite a few compile warnings
>> > > due to code like this, which it claims is unreachable (but isn't):
>> > > https://github.com/git/git/blob/v2.19.0-rc2/read-cache.c#L950-L955
>> >
>>
>> Wait a second - even if the compiler claims something (wrong)...
>> there a still 1+1/2 questions from my side:
>>
>>
>> int verify_path(const char *path, unsigned mode)
>> {
>> char c;
>> ^
>> /* Q1: should "c" be initialized like this: */
>> char c = *path;
>>
>> if (has_dos_drive_prefix(path))
>> return 0;
>>
>> goto inside;
>> /* Q2: and why do we need the "goto" here ? */
>> for (;;) {
>> if (!c)
>> return 1;
>> if (is_dir_sep(c)) {
>> inside:
>
> After some re-reading,
> I think that the "goto inside" was just hard to read
>
> Out of interest:
> would the following make the compiler happy ?
>
>
> diff --git a/read-cache.c b/read-cache.c
> index 49add63fe1..d574d58b9d 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -951,17 +951,15 @@ static int verify_dotfile(const char *rest, unsigned
> mode)
>
> int verify_path(const char *path, unsigned mode)
> {
> - char c;
> + char c = *path ? '/' : '\0';
>
> if (has_dos_drive_prefix(path))
> return 0;
>
> - goto inside;
> for (;;) {
> if (!c)
> return 1;
> if (is_dir_sep(c)) {
> -inside:
> if (protect_hfs) {
> if (is_hfs_dotgit(path))
> return 0;
I haven't tested (it's tedious) but yes, I can tell you SunCC won't
whine about this. I've only seen its unreachability detector get
confused about goto inside a loop, not the the sort of code you've
replaced this with.
We should not be appeasing these old compiler warnings in cases where
they're wrong. You can check out the CI output I linked to to see 10-20
cases in the codebase where SunCC is wrong about unreliability.
Whether we should just fix this for its own sake is another question.
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 24.11.18 um 23:07 schrieb Johannes Sixt: > I don't think that there is anything to fix. If you have a file with > CRLF in it, but you did not declare to Git that CRLF is the expected > end-of-line indicator, then the CR *is* trailing whitespace (because > the line ends at LF), and 'git diff' highlights it. Sure, it's correct to highlight it. But it doesn't highlight it in removed lines, just in added lines. I can see no good reason why removed and added lines should be treated differently. Let me give you two real-life examples: 1) If CR+LF line termination is used in a file, changing the content of a line (but not its termination) currently produces a diff like -something +something_new^M which causes the user to think he has changed the line ending (added a CR) although he didn't. 2) If someone/something unintentionally changes the line termination from CR+LF to LF, it doesn't show up in the diff: -something +something I don't think this behavior makes sense. At least it's IMHO not what users expect to see. They want to see what's really going on, not to get confused. Regards, Frank
For your Perusal
Dearest , Its really good to see your Prompt Response it show how Responsible and diligent you will be in handling the sum of $10.5 MILLION if placed in your care and for investment Purposes in your Country . These said Funds have being placed in Safekeeping in DEUTSCHE BANK in SOUTH AFRICA for long, under these very noble Transaction all you have to do is open an escrow and Offshore Non Resident account with the Bank online and I would instruct my Account Officer to credit your Account with the said $10.5 Million. After which you can then transfer by your self via your online details to your Designated Account in your Country. All the same send to me all details below: 1.Full Names. 2. Detailed Residential and Official Addresses. 3.Occupation. 4.Age. 5. Direct Cell Phone and Landline numbers . Looking forward to good and fruitful working Relationship and my regards to your FAMILY. Regards John William
[RFC PATCH 5/7] test-lib: use pkgsrc provided unzip for NetBSD
d98b2c5fce ("test-lib: on FreeBSD, look for unzip(1) in /usr/local/bin/",
2016-07-21) added an exception to the test suite for FreeBSD because the
tests assume functionality not provided by its base unzip tool.
NetBSD shares that limitation and provides a package that could be used
instead so all tests from t5003 succeed
Signed-off-by: Carlo Marcelo Arenas Belón
---
t/test-lib.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 6c6c0af7a1..2acb35f277 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1244,6 +1244,7 @@ test_lazy_prereq SANITY '
'
test FreeBSD != $uname_s || GIT_UNZIP=${GIT_UNZIP:-/usr/local/bin/unzip}
+test $uname_s = NetBSD && GIT_UNZIP=${GIT_UNZIP:-/usr/pkg/bin/unzip}
GIT_UNZIP=${GIT_UNZIP:-unzip}
test_lazy_prereq UNZIP '
"$GIT_UNZIP" -v
--
2.20.0.rc1
[RFC PATCH 7/7] config.mak.uname: use pkgsrc perl for NetBSD
otherwise will default to /usr/bin/perl which wouldn't normally exist Signed-off-by: Carlo Marcelo Arenas Belón --- config.mak.uname | 1 + 1 file changed, 1 insertion(+) diff --git a/config.mak.uname b/config.mak.uname index 59ce03819b..d2edb723f4 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -249,6 +249,7 @@ ifeq ($(uname_S),NetBSD) OLD_ICONV = UnfortunatelyYes BASIC_CFLAGS += -I/usr/pkg/include BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib + PERL_PATH = /usr/pkg/bin/perl USE_ST_TIMESPEC = YesPlease HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease -- 2.20.0.rc1
[RFC PATCH 6/7] t5004: use GNU tar to avoid known issues with BSD tar
56ee96572a ("t5004: resurrect original empty tar archive test", 2013-05-09)
added a test to try to detect and workaround issues with the standard tar
from BSD, but at least in NetBSD would be better to instead require GNU tar
which is available from pkgsrc
Signed-off-by: Carlo Marcelo Arenas Belón
---
t/t5004-archive-corner-cases.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
index ced44355ca..baafc553f8 100755
--- a/t/t5004-archive-corner-cases.sh
+++ b/t/t5004-archive-corner-cases.sh
@@ -31,6 +31,8 @@ test_lazy_prereq UNZIP_ZIP64_SUPPORT '
"$GIT_UNZIP" -v | grep ZIP64_SUPPORT
'
+test $uname_s = NetBSD && TAR="gtar"
+
# bsdtar/libarchive versions before 3.1.3 consider a tar file with a
# global pax header that is not followed by a file record as corrupt.
if "$TAR" tf "$TEST_DIRECTORY"/t5004/empty-with-pax-header.tar >/dev/null 2>&1
--
2.20.0.rc1
[RFC PATCH 2/7] t0301: remove trailing / for dir creation
the semantics of how mkdir -p should work, specially when using -m are not standard and in this case NetBSD will assume that the permision should not be changed, breaking the test -p is technically not needed either, but will be cleared in a future patch eventhough it could be considered an alternative fix Signed-off-by: Carlo Marcelo Arenas Belón --- t/t0301-credential-cache.sh | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh index fd92533acf..9529c612af 100755 --- a/t/t0301-credential-cache.sh +++ b/t/t0301-credential-cache.sh @@ -77,9 +77,9 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" ' test_expect_success 'use user socket if user directory exists' ' test_when_finished " git credential-cache exit && - rmdir \"\$HOME/.git-credential-cache/\" + rmdir \"\$HOME/.git-credential-cache\" " && - mkdir -p -m 700 "$HOME/.git-credential-cache/" && + mkdir -p -m 700 "$HOME/.git-credential-cache" && check approve cache <<-\EOF && protocol=https host=example.com @@ -92,10 +92,10 @@ test_expect_success 'use user socket if user directory exists' ' test_expect_success SYMLINKS 'use user socket if user directory is a symlink to a directory' ' test_when_finished " git credential-cache exit && - rmdir \"\$HOME/dir/\" && + rmdir \"\$HOME/dir\" && rm \"\$HOME/.git-credential-cache\" " && - mkdir -p -m 700 "$HOME/dir/" && + mkdir -p -m 700 "$HOME/dir" && ln -s "$HOME/dir" "$HOME/.git-credential-cache" && check approve cache <<-\EOF && protocol=https -- 2.20.0.rc1
[RFC PATCH 4/7] config.mak.uname: NetBSD uses old iconv interface
prevents the following warning : utf8.c: In function 'reencode_string_iconv': utf8.c:486:28: warning: passing argument 2 of 'iconv' from incompatible pointer type [-Wincompatible-pointer-types] size_t cnt = iconv(conv, , , , ); ^ In file included from git-compat-util.h:275:0, from utf8.c:1: /usr/include/iconv.h:46:8: note: expected 'const char ** restrict' but argument is of type 'char **' size_t iconv(iconv_t, const char ** __restrict, ^ it is set by optional configure at least since NetBSD 6.0 Signed-off-by: Carlo Marcelo Arenas Belón --- config.mak.uname | 1 + 1 file changed, 1 insertion(+) diff --git a/config.mak.uname b/config.mak.uname index 36c973c7e6..59ce03819b 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -246,6 +246,7 @@ ifeq ($(uname_S),NetBSD) ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2) NEEDS_LIBICONV = YesPlease endif + OLD_ICONV = UnfortunatelyYes BASIC_CFLAGS += -I/usr/pkg/include BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib USE_ST_TIMESPEC = YesPlease -- 2.20.0.rc1
[RFC PATCH 3/7] config.mak.uname: NetBSD uses BSD semantics with fread for directories
this "fixes" test 23 (proper error on directory "files") from t1308 other BSD (OpenBSD, MirBSD) likely also affected but they will be fixed in a different series the optional 'configure' sets this automatically and is probably what most users from this platform had been doing as a workaround Signed-off-by: Carlo Marcelo Arenas Belón --- config.mak.uname | 1 + 1 file changed, 1 insertion(+) diff --git a/config.mak.uname b/config.mak.uname index 3ee7da0e23..36c973c7e6 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -253,6 +253,7 @@ ifeq ($(uname_S),NetBSD) HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease PROCFS_EXECUTABLE_PATH = /proc/curproc/exe + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),AIX) DEFAULT_PAGER = more -- 2.20.0.rc1
[RFC PATCH 1/7] Documentation: update INSTALL for NetBSD
NetBSD added a BSD licensed reimplementation of GNU libintl to its base at least since release 4.0 (mid 2012) and git can be configured to build with it. Signed-off-by: Carlo Marcelo Arenas Belón --- INSTALL | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/INSTALL b/INSTALL index c39006e8e7..100718989f 100644 --- a/INSTALL +++ b/INSTALL @@ -154,11 +154,11 @@ Issues of note: git-gui, you can use NO_TCLTK. - A gettext library is used by default for localizing Git. The - primary target is GNU libintl, but the Solaris gettext - implementation also works. + primary target is GNU libintl, but the Solaris and NetBSD gettext + implementations also work. We need a gettext.h on the system for C code, gettext.sh (or - Solaris gettext(1)) for shell scripts, and libintl-perl for Perl + gettext(1) utility) for shell scripts, and libintl-perl for Perl programs. Set NO_GETTEXT to disable localization support and make Git only -- 2.20.0.rc1
[RFC PATCH 0/7] test: NetBSD support
Likely still missing changes as it only completes a run with a minimal number of dependencies but open for feedback Requires pkgsrc packages for gmake, perl, bash and curl and completes a run $ gmake SHELL_PATH=/usr/pkg/bin/bash NO_PYTHON=1 CURL_DIR=/usr/pkg test Carlo Marcelo Arenas Belón (7): Documentation: update INSTALL for NetBSD t0301: remove trailing / for dir creation config.mak.uname: NetBSD uses BSD semantics with fread for directories config.mak.uname: NetBSD uses old iconv interface test-lib: use pkgsrc provided unzip for NetBSD t5004: use GNU tar to avoid known issues with BSD tar config.mak.uname: use pkgsrc perl for NetBSD INSTALL | 6 +++--- config.mak.uname| 3 +++ t/t0301-credential-cache.sh | 8 t/t5004-archive-corner-cases.sh | 2 ++ t/test-lib.sh | 1 + 5 files changed, 13 insertions(+), 7 deletions(-) -- 2.20.0.rc1
Re: [PATCH] files-backend.c: fix build error on Solaris
On Sun, Nov 25, 2018 at 11:19 AM Carlo Arenas wrote: > > Signed-off-by: Carlo Marcelo Arenas Belón > > clang with -Wpedantic also catch this (at least with Apple LLVM > version 10.0.0); recent versions of gcc also include that flag and at > least 8.2.0 shows a warning for it, so it might be worth adding it to > developer mode (maybe under the pedantic DEVOPTS), would this be > something I should be adding?, what are the expectations around > warnings and compilers? make DEVELOPER=1 DEVOPTS=pedantic (with either gcc 7.3.0 or 8.2.0) can already catch this. I have no comment if this pedantic should be part of default DEVELOPER=1. But at least in controlled environments like Travis, we could turn it on to catch more. -- Duy
Re: [PATCH] files-backend.c: fix build error on Solaris
Signed-off-by: Carlo Marcelo Arenas Belón clang with -Wpedantic also catch this (at least with Apple LLVM version 10.0.0); recent versions of gcc also include that flag and at least 8.2.0 shows a warning for it, so it might be worth adding it to developer mode (maybe under the pedantic DEVOPTS), would this be something I should be adding?, what are the expectations around warnings and compilers? Carlo
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Sun, Nov 25, 2018 at 05:28:35AM +0100, Torsten Bögershausen wrote:
> On Sat, Nov 24, 2018 at 08:33:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >
> > On Wed, Sep 05 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> > > On Wed, Sep 05 2018, Eric Sunshine wrote:
>
> []
>
> > > SunCC used to be ahead of GCC & Clang when it came to certain classes of
> > > warnings, but e.g. now everything it complains about is because it
> > > doesn't understand C as well, e.g. we have quite a few compile warnings
> > > due to code like this, which it claims is unreachable (but isn't):
> > > https://github.com/git/git/blob/v2.19.0-rc2/read-cache.c#L950-L955
> >
>
> Wait a second - even if the compiler claims something (wrong)...
> there a still 1+1/2 questions from my side:
>
>
> int verify_path(const char *path, unsigned mode)
> {
> char c;
>^
> /* Q1: should "c" be initialized like this: */
> char c = *path;
>
> if (has_dos_drive_prefix(path))
> return 0;
>
> goto inside;
> /* Q2: and why do we need the "goto" here ? */
> for (;;) {
> if (!c)
> return 1;
> if (is_dir_sep(c)) {
> inside:
After some re-reading,
I think that the "goto inside" was just hard to read
Out of interest:
would the following make the compiler happy ?
diff --git a/read-cache.c b/read-cache.c
index 49add63fe1..d574d58b9d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -951,17 +951,15 @@ static int verify_dotfile(const char *rest, unsigned mode)
int verify_path(const char *path, unsigned mode)
{
- char c;
+ char c = *path ? '/' : '\0';
if (has_dos_drive_prefix(path))
return 0;
- goto inside;
for (;;) {
if (!c)
return 1;
if (is_dir_sep(c)) {
-inside:
if (protect_hfs) {
if (is_hfs_dotgit(path))
return 0;
