* [PATCH v2 0/1] Support `git reset --stdin`
From: Johannes Schindelin @ 2017-01-27 12:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Jeff King
In-Reply-To: <cover.1476202100.git.johannes.schindelin@gmx.de>
This feature was missing, and made it cumbersome for third-party
tools to reset a lot of paths in one go.
Support for --stdin has been added, following builtin/checkout-index.c's
example.
Changes since v1:
- adjusted commit message to explain why we read everything before
resetting
- fixed synopsis (`--stdin` does not work with `-- <path>...`)
- avoid handling patch_mode twice
- use PATHSPEC_LITERAL_PATH to avoid interpreting input as wildcards
Johannes Schindelin (1):
reset: support the --stdin option
Documentation/git-reset.txt | 9 ++++++++
builtin/reset.c | 54 ++++++++++++++++++++++++++++++++++++++++++++-
t/t7107-reset-stdin.sh | 33 +++++++++++++++++++++++++++
3 files changed, 95 insertions(+), 1 deletion(-)
create mode 100755 t/t7107-reset-stdin.sh
base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
Published-As: https://github.com/dscho/git/releases/tag/reset-stdin-v2
Fetch-It-Via: git fetch https://github.com/dscho/git reset-stdin-v2
Interdiff vs v1:
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 533ef69f91..abb71bb805 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -8,8 +8,9 @@ git-reset - Reset current HEAD to the specified state
SYNOPSIS
--------
[verse]
-'git reset' [-q] [--stdin [-z]] [<tree-ish>] [--] <paths>...
+'git reset' [-q] [<tree-ish>] [--] <paths>...
'git reset' (--patch | -p) [<tree-ish>] [--] [<paths>...]
+'git reset' [-q] [--stdin [-z]] [<tree-ish>]
'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] [<commit>]
DESCRIPTION
diff --git a/builtin/reset.c b/builtin/reset.c
index 6de3936aed..1d3075b7ee 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -26,7 +26,8 @@
static const char * const git_reset_usage[] = {
N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
- N_("git reset [-q] [--stdin [-z]] [<tree-ish>] [--] <paths>..."),
+ N_("git reset [-q] [<tree-ish>] [--] <paths>..."),
+ N_("git reset [-q] [--stdin [-z]] [<tree-ish>]"),
N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),
NULL
};
@@ -317,9 +318,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (pathspec.nr)
die(_("--stdin is incompatible with path arguments"));
- if (patch_mode)
- flags |= PATHSPEC_PREFIX_ORIGIN;
-
while (getline_fn(&buf, stdin) != EOF) {
if (!nul_term_line && buf.buf[0] == '"') {
strbuf_reset(&unquoted);
@@ -336,8 +334,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
stdin_paths[stdin_nr++] = NULL;
+ flags |= PATHSPEC_LITERAL_PATH;
parse_pathspec(&pathspec, 0, flags, prefix,
(const char **)stdin_paths);
+
} else if (nul_term_line)
die(_("-z requires --stdin"));
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply
* [PATCH] help: correct behavior for is_executable on Windows
From: Johannes Schindelin @ 2017-01-27 13:50 UTC (permalink / raw)
To: git; +Cc: Heiko Voigt, Junio C Hamano
From: Heiko Voigt <hvoigt@hvoigt.net>
The previous implementation said that the filesystem information on
Windows is not reliable to determine whether a file is executable. To
gather this information it was peeking into the first two bytes of a
file to see whether it looks executable.
Apart from the fact that on Windows executables are defined as such by
their extension (and Git has special code to support "executing" scripts
when they have a she-bang line) it leads to serious performance
problems: not only do we have to open many files now, it gets even
slower when a virus scanner is running.
See the following measurements (in seconds) as an example, where we
execute a simple program that simply lists the directory contents and
calls open() on every listed file:
With virus scanner running (coldcache):
$ ./a.exe /libexec/git-core/
before open (git-add.exe): 0.000000
after open (git-add.exe): 0.412873
before open (git-annotate.exe): 0.000175
after open (git-annotate.exe): 0.397925
before open (git-apply.exe): 0.000243
after open (git-apply.exe): 0.399996
before open (git-archive.exe): 0.000147
after open (git-archive.exe): 0.397783
before open (git-bisect--helper.exe): 0.000160
after open (git-bisect--helper.exe): 0.397700
before open (git-blame.exe): 0.000160
after open (git-blame.exe): 0.399136
...
With virus scanner running (hotcache):
$ ./a.exe /libexec/git-core/
before open (git-add.exe): 0.000000
after open (git-add.exe): 0.000325
before open (git-annotate.exe): 0.000229
after open (git-annotate.exe): 0.000177
before open (git-apply.exe): 0.000167
after open (git-apply.exe): 0.000150
before open (git-archive.exe): 0.000154
after open (git-archive.exe): 0.000156
before open (git-bisect--helper.exe): 0.000132
after open (git-bisect--helper.exe): 0.000180
before open (git-blame.exe): 0.000718
after open (git-blame.exe): 0.000724
...
With this patch I get:
$ time git help git
Launching default browser to display HTML ...
real 0m8.723s
user 0m0.000s
sys 0m0.000s
and without
$ time git help git
Launching default browser to display HTML ...
real 1m37.734s
user 0m0.000s
sys 0m0.031s
both tests with cold cache and giving the machine some time to settle
down after restart.
Signed-off-by: Heiko Voigt <heiko.voigt@mahr.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/help-is-exe-v1
Fetch-It-Via: git fetch https://github.com/dscho/git help-is-exe-v1
help.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/help.c b/help.c
index 53e2a67e00..bc6cd19cf3 100644
--- a/help.c
+++ b/help.c
@@ -105,7 +105,22 @@ static int is_executable(const char *name)
return 0;
#if defined(GIT_WINDOWS_NATIVE)
-{ /* cannot trust the executable bit, peek into the file instead */
+ /*
+ * On Windows there is no executable bit. The file extension
+ * indicates whether it can be run as an executable, and Git
+ * has special-handling to detect scripts and launch them
+ * through the indicated script interpreter. We test for the
+ * file extension first because virus scanners may make
+ * it quite expensive to open many files.
+ */
+ if (ends_with(name, ".exe"))
+ return S_IXUSR;
+
+{
+ /*
+ * Now that we know it does not have an executable extension,
+ * peek into the file instead.
+ */
char buf[3] = { 0 };
int n;
int fd = open(name, O_RDONLY);
@@ -113,8 +128,8 @@ static int is_executable(const char *name)
if (fd >= 0) {
n = read(fd, buf, 2);
if (n == 2)
- /* DOS executables start with "MZ" */
- if (!strcmp(buf, "#!") || !strcmp(buf, "MZ"))
+ /* look for a she-bang */
+ if (!strcmp(buf, "#!"))
st.st_mode |= S_IXUSR;
close(fd);
}
base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* Re: [PATCH] test-lib: on FreeBSD, look for unzip(1) in /usr/local/bin/
From: Johannes Schindelin @ 2017-01-27 14:03 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Junio C Hamano
In-Reply-To: <20160721171025.GA30979@plume>
Hi Junio,
On Thu, 21 Jul 2016, Eric Wong wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> > The common work-around is to install Info-Zip on FreeBSD, into
> > /usr/local/bin/.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Thanks, t5003 now works out-of-the-box.
> Tested with Info-ZIP unzip installed and uninstalled.
>
> Tested-by: Eric Wong <e@80x24.org>
Did you forget about this?
Ciao,
Johannes
^ permalink raw reply
* [PATCH v3] t/Makefile: add a rule to re-run previously-failed tests
From: Johannes Schindelin @ 2017-01-27 14:17 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Sverre Rabbelier,
Ævar Arnfjörð Bjarmason, Matthieu Moy
In-Reply-To: <0dfa96b17edfe84ba19c7e57fe0b017c77943e0c.1472478285.git.johannes.schindelin@gmx.de>
This patch automates the process of determinig which tests failed
previously and re-running them.
While developing patch series, it is a good practice to run the test
suite from time to time, just to make sure that obvious bugs are caught
early. With complex patch series, it is common to run `make -j15 -k
test`, i.e. run the tests in parallel and *not* stop at the first
failing test but continue. This has the advantage of identifying
possibly multiple problems in one big test run.
It is particularly important to reduce the turn-around time thusly on
Windows, where the test suite spends 45 minutes on the computer on which
this patch was developed.
It is the most convenient way to determine which tests failed after
running the entire test suite, in parallel, to look for left-over "trash
directory.t*" subdirectories in the t/ subdirectory. However, those
directories might live outside t/ when overridden using the
--root=<directory> option, to which the Makefile has no access. The next
best method is to grep explicitly for failed tests in the test-results/
directory, which the Makefile *can* access.
Please note that the often-recommended `prove` tool requires Perl, and
that opens a whole new can of worms on Windows. As no native Windows Perl
comes with Subversion bindings, we have to use a Perl in Git for Windows
that uses the POSIX emulation layer named MSYS2 (which is a portable
version of Cygwin). When using this emulation layer under stress, e.g.
when running massively-parallel tests, unexplicable crashes occur quite
frequently, and instead of having a solution to the original problem, the
developer now has an additional, quite huge problem. For that reason, this
developer rejected `prove` as a solution and went with this patch instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/failing-tests-v3
Fetch-It-Via: git fetch https://github.com/dscho/git failing-tests-v3
Interdiff vs v2:
diff --git a/t/Makefile b/t/Makefile
index 8aa6a72a70..1bb06c36f2 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -37,9 +37,8 @@ test: pre-clean $(TEST_LINT)
failed:
@failed=$$(cd '$(TEST_RESULTS_DIRECTORY_SQ)' && \
- grep -l '^failed [1-9]' $$(ls -t *.counts | \
- sed 'G;h;/^\(t[^.]*\)-[0-9]*\..*\n\1-[0-9]*\./d;P;d') | \
- sed -n 's/-[0-9]*\.counts$$/.sh/p') && \
+ grep -l '^failed [1-9]' *.counts | \
+ sed -n 's/\.counts$$/.sh/p') && \
test -z "$$failed" || $(MAKE) $$failed
prove: pre-clean $(TEST_LINT)
t/Makefile | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/t/Makefile b/t/Makefile
index d613935f14..1bb06c36f2 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -35,6 +35,12 @@ all: $(DEFAULT_TEST_TARGET)
test: pre-clean $(TEST_LINT)
$(MAKE) aggregate-results-and-cleanup
+failed:
+ @failed=$$(cd '$(TEST_RESULTS_DIRECTORY_SQ)' && \
+ grep -l '^failed [1-9]' *.counts | \
+ sed -n 's/\.counts$$/.sh/p') && \
+ test -z "$$failed" || $(MAKE) $$failed
+
prove: pre-clean $(TEST_LINT)
@echo "*** prove ***"; $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
$(MAKE) clean-except-prove-cache
base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* Re: [PATCH 2/2] use absolute_pathdup()
From: René Scharfe @ 2017-01-27 15:39 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701271121120.3469@virtualbox>
Hi Dscho,
Am 27.01.2017 um 11:21 schrieb Johannes Schindelin:
> On Thu, 26 Jan 2017, René Scharfe wrote:
>> Apply the symantic patch for converting callers that duplicate the
>
> s/symantic/semantic/
thank you! I wonder where this came from. And where my spellchecker
went without as much as a farewell. Reinstalled it now..
René
^ permalink raw reply
* Re: [PATCH v2 1/1] reset: support the --stdin option
From: Jeff King @ 2017-01-27 17:04 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Jakub Narębski
In-Reply-To: <3b0bad26045ed175f40f050e15b65cb6a8f2368c.1485520718.git.johannes.schindelin@gmx.de>
On Fri, Jan 27, 2017 at 01:38:55PM +0100, Johannes Schindelin wrote:
> Just like with other Git commands, this option makes it read the paths
> from the standard input. It comes in handy when resetting many, many
> paths at once and wildcards are not an option (e.g. when the paths are
> generated by a tool).
>
> Note: we first parse the entire list and perform the actual reset action
> only in a second phase. Not only does this make things simpler, it also
> helps performance, as do_diff_cache() traverses the index and the
> (sorted) pathspecs in simultaneously to avoid unnecessary lookups.
This looks OK to me. At first I wondered if using PATHSPEC_LITERAL_PATH
was consistent with other "--stdin" tools. I think it mostly is (or at
least consistent with checkout-index). The exception is "rev-list
--stdin", but that's probably fine. It is taking rev-list arguments in
the first place, not a list of paths.
A few minor suggestions:
> +--stdin::
> + Instead of taking list of paths from the command line,
> + read list of paths from the standard input. Paths are
> + separated by LF (i.e. one path per line) by default.
> +
> +-z::
> + Only meaningful with `--stdin`; paths are separated with
> + NUL character instead of LF.
Is it worth clarifying that these are paths, not pathspecs? The word
"paths" is used to refer to the pathspecs on the command-line elsewhere
in the document.
It might also be worth mentioning the quoting rules for the non-z case.
> @@ -267,7 +270,9 @@ static int reset_refs(const char *rev, const struct object_id *oid)
> int cmd_reset(int argc, const char **argv, const char *prefix)
> {
> int reset_type = NONE, update_ref_status = 0, quiet = 0;
> - int patch_mode = 0, unborn;
> + int patch_mode = 0, nul_term_line = 0, read_from_stdin = 0, unborn;
> + char **stdin_paths = NULL;
> + int stdin_nr = 0, stdin_alloc = 0;
This list is a good candidate for an argv_array, I think. So:
struct argv_array stdin_paths = ARGV_ARRAY_INIT;
> + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> + stdin_paths[stdin_nr++] = xstrdup(buf.buf);
argv_array_push(&stdin_paths, buf.buf);
> + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> + stdin_paths[stdin_nr++] = NULL;
This goes away because argv_array handles it for you.
> + if (stdin_paths) {
> + while (stdin_nr)
> + free(stdin_paths[--stdin_nr]);
> + free(stdin_paths);
> + }
argv_array_clear(&stdin_paths);
> @@ -295,6 +304,43 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
> PARSE_OPT_KEEP_DASHDASH);
> parse_args(&pathspec, argv, prefix, patch_mode, &rev);
>
> + if (read_from_stdin) {
> + strbuf_getline_fn getline_fn = nul_term_line ?
> + strbuf_getline_nul : strbuf_getline_lf;
> + int flags = PATHSPEC_PREFER_FULL |
> + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP;
You set two flags here...
> + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> + stdin_paths[stdin_nr++] = NULL;
> + flags |= PATHSPEC_LITERAL_PATH;
> + parse_pathspec(&pathspec, 0, flags, prefix,
> + (const char **)stdin_paths);
...and then one more here. They all seem to be set unconditionally, and
we never look at "flags" between the two lines. I think it would be more
obvious to set them all in the same place.
> + } else if (nul_term_line)
> + die(_("-z requires --stdin"));
> +
Hmm, there's our brace question coming up again. :)
> diff --git a/t/t7107-reset-stdin.sh b/t/t7107-reset-stdin.sh
> new file mode 100755
> index 0000000000..997dc42dd2
> --- /dev/null
> +++ b/t/t7107-reset-stdin.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +
> +test_description='reset --stdin'
Here are a few things we might want to test that I didn't see covered:
- feeding path X does not reset path Y
- de-quoting in the non-z case
-Peff
^ permalink raw reply
* Re: [PATCH v3] t/Makefile: add a rule to re-run previously-failed tests
From: Jeff King @ 2017-01-27 17:07 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Sverre Rabbelier,
Ævar Arnfjörð Bjarmason, Matthieu Moy
In-Reply-To: <85044791cfcba35c1ad3d8138051f3f075cb0646.1485526641.git.johannes.schindelin@gmx.de>
On Fri, Jan 27, 2017 at 03:17:36PM +0100, Johannes Schindelin wrote:
> This patch automates the process of determinig which tests failed
> previously and re-running them.
s/determinig/determining/
Patch otherwise looks good, and I'm happy to be rid of the sed
complexity from v2.
-Peff
^ permalink raw reply
* show all merge conflicts
From: Michael Spiegel @ 2017-01-27 16:56 UTC (permalink / raw)
To: git
Hi folks,
I'm trying to determine whether a merge required a conflict to resolve
after the merge has occurred. The git book has some advice
(https://git-scm.com/book/en/v2/Git-Tools-Advanced-Merging) to use
`git show` on the merge commit or use `git log --cc -p -1`. These
strategies work when the merge conflict was resolved with a change
that is different from either parent. When the conflict is resolved
with a change that is the same as one of the parents, then these
commands are indistinguishable from a merge that did not conflict. Is
it possible to distinguish between a conflict-free merge and a merge
conflict that is resolved by with the changes from one the parents?
Thanks,
--Michael
^ permalink raw reply
* [PATCH v4] t/Makefile: add a rule to re-run previously-failed tests
From: Johannes Schindelin @ 2017-01-27 17:21 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Sverre Rabbelier,
Ævar Arnfjörð Bjarmason, Matthieu Moy
In-Reply-To: <85044791cfcba35c1ad3d8138051f3f075cb0646.1485526641.git.johannes.schindelin@gmx.de>
This patch automates the process of determining which tests failed
previously and re-running them.
While developing patch series, it is a good practice to run the test
suite from time to time, just to make sure that obvious bugs are caught
early. With complex patch series, it is common to run `make -j15 -k
test`, i.e. run the tests in parallel and *not* stop at the first
failing test but continue. This has the advantage of identifying
possibly multiple problems in one big test run.
It is particularly important to reduce the turn-around time thusly on
Windows, where the test suite spends 45 minutes on the computer on which
this patch was developed.
It is the most convenient way to determine which tests failed after
running the entire test suite, in parallel, to look for left-over "trash
directory.t*" subdirectories in the t/ subdirectory. However, those
directories might live outside t/ when overridden using the
--root=<directory> option, to which the Makefile has no access. The next
best method is to grep explicitly for failed tests in the test-results/
directory, which the Makefile *can* access.
Please note that the often-recommended `prove` tool requires Perl, and
that opens a whole new can of worms on Windows. As no native Windows Perl
comes with Subversion bindings, we have to use a Perl in Git for Windows
that uses the POSIX emulation layer named MSYS2 (which is a portable
version of Cygwin). When using this emulation layer under stress, e.g.
when running massively-parallel tests, unexplicable crashes occur quite
frequently, and instead of having a solution to the original problem, the
developer now has an additional, quite huge problem. For that reason, this
developer rejected `prove` as a solution and went with this patch instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/failing-tests-v4
Fetch-It-Via: git fetch https://github.com/dscho/git failing-tests-v4
t/Makefile | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/t/Makefile b/t/Makefile
index d613935f14..1bb06c36f2 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -35,6 +35,12 @@ all: $(DEFAULT_TEST_TARGET)
test: pre-clean $(TEST_LINT)
$(MAKE) aggregate-results-and-cleanup
+failed:
+ @failed=$$(cd '$(TEST_RESULTS_DIRECTORY_SQ)' && \
+ grep -l '^failed [1-9]' *.counts | \
+ sed -n 's/\.counts$$/.sh/p') && \
+ test -z "$$failed" || $(MAKE) $$failed
+
prove: pre-clean $(TEST_LINT)
@echo "*** prove ***"; $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
$(MAKE) clean-except-prove-cache
base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* Re: [PATCH v2] git-p4: Fix git-p4.mapUser on Windows
From: Junio C Hamano @ 2017-01-27 17:33 UTC (permalink / raw)
To: Luke Diamand, Lars Schneider; +Cc: git, George Vanburgh
In-Reply-To: <01020159d4ea6861-fce45d61-8be5-49b0-ab4e-d161b9429795-000000@eu-west-1.amazonses.com>
George Vanburgh <george@vanburgh.me> writes:
> From: George Vanburgh <gvanburgh@bloomberg.net>
>
> When running git-p4 on Windows, with multiple git-p4.mapUser entries in
> git config - no user mappings are applied to the generated repository.
> ...
> Using splitlines solves this issue, by splitting config on all
> typical delimiters ('\n', '\r\n' etc.)
Luke, Lars, this version seems to be in line with the conclusion of
your earlier reviews, e.g.
<CAE5ih7_+Vc9oqKdjo8h2vgZPup4pto9wd=sBb=W6hCs4tuW2Jg@mail.gmail.com>
Even though it looks OK to my eyes, I'll wait for Acks or further
refinement suggestions from either of you two before acting on this
patch.
Thanks.
> Signed-off-by: George Vanburgh <gvanburgh@bloomberg.net>
> ---
> git-p4.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index f427bf6..b66f68b 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -656,7 +656,7 @@ def gitConfigInt(key):
> def gitConfigList(key):
> if not _gitConfig.has_key(key):
> s = read_pipe(["git", "config", "--get-all", key], ignore_error=True)
> - _gitConfig[key] = s.strip().split(os.linesep)
> + _gitConfig[key] = s.strip().splitlines()
> if _gitConfig[key] == ['']:
> _gitConfig[key] = []
> return _gitConfig[key]
>
> --
> https://github.com/git/git/pull/319
^ permalink raw reply
* Re: [PATCH v2 1/1] reset: support the --stdin option
From: Johannes Schindelin @ 2017-01-27 17:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Jakub Narębski
In-Reply-To: <20170127170422.lvpghp6jdud37zxx@sigill.intra.peff.net>
Hi Peff,
On Fri, 27 Jan 2017, Jeff King wrote:
> On Fri, Jan 27, 2017 at 01:38:55PM +0100, Johannes Schindelin wrote:
>
> A few minor suggestions:
>
> > +--stdin::
> > + Instead of taking list of paths from the command line,
> > + read list of paths from the standard input. Paths are
> > + separated by LF (i.e. one path per line) by default.
> > +
> > +-z::
> > + Only meaningful with `--stdin`; paths are separated with
> > + NUL character instead of LF.
>
> Is it worth clarifying that these are paths, not pathspecs? The word
> "paths" is used to refer to the pathspecs on the command-line elsewhere
> in the document.
>
> It might also be worth mentioning the quoting rules for the non-z case.
I think this would be overkill. In reality, --stdin without -z does not
make much sense, anyway.
If you feel strongly about it, I encourage you to submit a follow-up
patch.
The rest of your suggestions have been implemented in v3.
Ciao,
Johannes
^ permalink raw reply
* [PATCH v3 1/1] reset: support the --stdin option
From: Johannes Schindelin @ 2017-01-27 17:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Jeff King
In-Reply-To: <cover.1485538197.git.johannes.schindelin@gmx.de>
Just like with other Git commands, this option makes it read the paths
from the standard input. It comes in handy when resetting many, many
paths at once and wildcards are not an option (e.g. when the paths are
generated by a tool).
Note: we first parse the entire list and perform the actual reset action
only in a second phase. Not only does this make things simpler, it also
helps performance, as do_diff_cache() traverses the index and the
(sorted) pathspecs in simultaneously to avoid unnecessary lookups.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/git-reset.txt | 10 ++++++++++
builtin/reset.c | 47 ++++++++++++++++++++++++++++++++++++++++++++-
t/t7107-reset-stdin.sh | 33 +++++++++++++++++++++++++++++++
3 files changed, 89 insertions(+), 1 deletion(-)
create mode 100755 t/t7107-reset-stdin.sh
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 25432d9257..d319ed9b20 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -10,6 +10,7 @@ SYNOPSIS
[verse]
'git reset' [-q] [<tree-ish>] [--] <paths>...
'git reset' (--patch | -p) [<tree-ish>] [--] [<paths>...]
+'git reset' [-q] [--stdin [-z]] [<tree-ish>]
'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] [<commit>]
DESCRIPTION
@@ -97,6 +98,15 @@ OPTIONS
--quiet::
Be quiet, only report errors.
+--stdin::
+ Instead of taking list of paths from the command line,
+ read list of paths from the standard input. The paths are
+ read verbatim, i.e. not handled as pathspecs. Paths are
+ separated by LF (i.e. one path per line) by default.
+
+-z::
+ Only meaningful with `--stdin`; paths are separated with
+ NUL character instead of LF.
EXAMPLES
--------
diff --git a/builtin/reset.c b/builtin/reset.c
index 8ab915bfcb..fe7723c179 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -21,10 +21,14 @@
#include "parse-options.h"
#include "unpack-trees.h"
#include "cache-tree.h"
+#include "strbuf.h"
+#include "quote.h"
+#include "argv-array.h"
static const char * const git_reset_usage[] = {
N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
N_("git reset [-q] [<tree-ish>] [--] <paths>..."),
+ N_("git reset [-q] [--stdin [-z]] [<tree-ish>]"),
N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),
NULL
};
@@ -267,7 +271,8 @@ static int reset_refs(const char *rev, const struct object_id *oid)
int cmd_reset(int argc, const char **argv, const char *prefix)
{
int reset_type = NONE, update_ref_status = 0, quiet = 0;
- int patch_mode = 0, unborn;
+ int patch_mode = 0, nul_term_line = 0, read_from_stdin = 0, unborn;
+ struct argv_array stdin_paths = ARGV_ARRAY_INIT;
const char *rev;
struct object_id oid;
struct pathspec pathspec;
@@ -286,6 +291,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
OPT_BOOL('N', "intent-to-add", &intent_to_add,
N_("record only the fact that removed paths will be added later")),
+ OPT_BOOL('z', NULL, &nul_term_line,
+ N_("paths are separated with NUL character")),
+ OPT_BOOL(0, "stdin", &read_from_stdin,
+ N_("read paths from <stdin>")),
OPT_END()
};
@@ -295,6 +304,40 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
PARSE_OPT_KEEP_DASHDASH);
parse_args(&pathspec, argv, prefix, patch_mode, &rev);
+ if (read_from_stdin) {
+ strbuf_getline_fn getline_fn = nul_term_line ?
+ strbuf_getline_nul : strbuf_getline_lf;
+ int flags = PATHSPEC_PREFER_FULL |
+ PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP;
+ struct strbuf buf = STRBUF_INIT;
+ struct strbuf unquoted = STRBUF_INIT;
+
+ if (patch_mode)
+ die(_("--stdin is incompatible with --patch"));
+
+ if (pathspec.nr)
+ die(_("--stdin is incompatible with path arguments"));
+
+ while (getline_fn(&buf, stdin) != EOF) {
+ if (!nul_term_line && buf.buf[0] == '"') {
+ strbuf_reset(&unquoted);
+ if (unquote_c_style(&unquoted, buf.buf, NULL))
+ die(_("line is badly quoted"));
+ strbuf_swap(&buf, &unquoted);
+ }
+ argv_array_push(&stdin_paths, buf.buf);
+ strbuf_reset(&buf);
+ }
+ strbuf_release(&unquoted);
+ strbuf_release(&buf);
+
+ flags |= PATHSPEC_LITERAL_PATH;
+ parse_pathspec(&pathspec, 0, flags, prefix,
+ stdin_paths.argv);
+
+ } else if (nul_term_line)
+ die(_("-z requires --stdin"));
+
unborn = !strcmp(rev, "HEAD") && get_sha1("HEAD", oid.hash);
if (unborn) {
/* reset on unborn branch: treat as reset to empty tree */
@@ -385,5 +428,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (!pathspec.nr)
remove_branch_state();
+ argv_array_clear(&stdin_paths);
+
return update_ref_status;
}
diff --git a/t/t7107-reset-stdin.sh b/t/t7107-reset-stdin.sh
new file mode 100755
index 0000000000..997dc42dd2
--- /dev/null
+++ b/t/t7107-reset-stdin.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='reset --stdin'
+
+. ./test-lib.sh
+
+test_expect_success 'reset --stdin' '
+ test_commit hello &&
+ git rm hello.t &&
+ test -z "$(git ls-files hello.t)" &&
+ echo hello.t | git reset --stdin &&
+ test hello.t = "$(git ls-files hello.t)"
+'
+
+test_expect_success 'reset --stdin -z' '
+ test_commit world &&
+ git rm hello.t world.t &&
+ test -z "$(git ls-files hello.t world.t)" &&
+ printf world.tQworld.tQhello.tQ | q_to_nul | git reset --stdin -z &&
+ printf "hello.t\nworld.t\n" >expect &&
+ git ls-files >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--stdin requires --mixed' '
+ echo hello.t >list &&
+ test_must_fail git reset --soft --stdin <list &&
+ test_must_fail git reset --hard --stdin <list &&
+ git reset --mixed --stdin <list
+'
+
+test_done
+
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* [PATCH v3 0/1] Support `git reset --stdin`
From: Johannes Schindelin @ 2017-01-27 17:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Jeff King
In-Reply-To: <cover.1485520718.git.johannes.schindelin@gmx.de>
This feature was missing, and made it cumbersome for third-party
tools to reset a lot of paths in one go.
Support for --stdin has been added, following builtin/checkout-index.c's
example.
Changes since v2:
- the documentation clarifies that --stdin does not treat the input as
pathspecs
- the code now uses struct argv_array instead of rolling its own
Johannes Schindelin (1):
reset: support the --stdin option
Documentation/git-reset.txt | 10 ++++++++++
builtin/reset.c | 47 ++++++++++++++++++++++++++++++++++++++++++++-
t/t7107-reset-stdin.sh | 33 +++++++++++++++++++++++++++++++
3 files changed, 89 insertions(+), 1 deletion(-)
create mode 100755 t/t7107-reset-stdin.sh
base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
Published-As: https://github.com/dscho/git/releases/tag/reset-stdin-v3
Fetch-It-Via: git fetch https://github.com/dscho/git reset-stdin-v3
Interdiff vs v2:
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index abb71bb805..d319ed9b20 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -100,7 +100,8 @@ OPTIONS
--stdin::
Instead of taking list of paths from the command line,
- read list of paths from the standard input. Paths are
+ read list of paths from the standard input. The paths are
+ read verbatim, i.e. not handled as pathspecs. Paths are
separated by LF (i.e. one path per line) by default.
-z::
diff --git a/builtin/reset.c b/builtin/reset.c
index 1d3075b7ee..fe7723c179 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -23,6 +23,7 @@
#include "cache-tree.h"
#include "strbuf.h"
#include "quote.h"
+#include "argv-array.h"
static const char * const git_reset_usage[] = {
N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
@@ -271,8 +272,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
{
int reset_type = NONE, update_ref_status = 0, quiet = 0;
int patch_mode = 0, nul_term_line = 0, read_from_stdin = 0, unborn;
- char **stdin_paths = NULL;
- int stdin_nr = 0, stdin_alloc = 0;
+ struct argv_array stdin_paths = ARGV_ARRAY_INIT;
const char *rev;
struct object_id oid;
struct pathspec pathspec;
@@ -325,18 +325,15 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
die(_("line is badly quoted"));
strbuf_swap(&buf, &unquoted);
}
- ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
- stdin_paths[stdin_nr++] = xstrdup(buf.buf);
+ argv_array_push(&stdin_paths, buf.buf);
strbuf_reset(&buf);
}
strbuf_release(&unquoted);
strbuf_release(&buf);
- ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
- stdin_paths[stdin_nr++] = NULL;
flags |= PATHSPEC_LITERAL_PATH;
parse_pathspec(&pathspec, 0, flags, prefix,
- (const char **)stdin_paths);
+ stdin_paths.argv);
} else if (nul_term_line)
die(_("-z requires --stdin"));
@@ -431,11 +428,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (!pathspec.nr)
remove_branch_state();
- if (stdin_paths) {
- while (stdin_nr)
- free(stdin_paths[--stdin_nr]);
- free(stdin_paths);
- }
+ argv_array_clear(&stdin_paths);
return update_ref_status;
}
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply
* Re: SubmittingPatches: drop temporal reference for PGP signing
From: Junio C Hamano @ 2017-01-27 17:43 UTC (permalink / raw)
To: Cornelius Weig
Cc: Philip Oakley, Stefan Beller, Johannes Sixt,
bitte.keine.werbung.einwerfen, git
In-Reply-To: <60e9abdf-cc37-33d8-e7eb-8a3370ffe1cc@tngtech.com>
Cornelius Weig <cornelius.weig@tngtech.com> writes:
> -Do not PGP sign your patch, -at least for now-. Most likely, your (...)
> +Do not PGP sign your patch. Most likely, your maintainer or other (...)
It has been quite a while since we wrote that "at least for now", so
it probably makes sense to drop it.
>> Maybe even s/by signing off/by adding your Signed-off-by:/ to be sure
>> that the reader knows that it is _their certification_ that is being
>> sought. Even if it does double up on the 'your'.
>
> I don't think doubling on the 'your' will make the heading clearer. The
> main intention of this change is to avoid mixups with pgp-signing and
> that would IMHO not improve by that.
> Besides, I find the colon in the heading a bit awkward. Is the following
> version as expressive as with the colon?
>
> -(5) Sign your work
> +(5) Certify your work by adding Signed-off-by
I am leaning towards agreeing with Philip, and my gut feeling says
the presense of the colon, would help a lot. It would visually
click to readers what we are talking about.
At the same time, I think ending a section header with a colon would
be seen as "awkward". I would have written it more like this to
avoid it:
... by adding "Signed-off-by: " line
I personally do not mind having "your" there, either, but I can see
that a section header wants to be kept shorter.
^ permalink raw reply
* Re: [PATCH v3 4/4] urlmatch: allow globbing for the URL host part
From: Junio C Hamano @ 2017-01-27 17:45 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Patrick Steinhardt, Philip Oakley
In-Reply-To: <20170127062142.GA760@pks-pc>
Patrick Steinhardt <patrick.steinhardt@elego.de> writes:
>> This is probably a useful improvement.
>>
>> Having said that, when I mentioned "glob", I meant to also support
>> something like this:
>>
>> https://www[1-4].ibm.com/
>
> The problem with additional extended syntax like proposed by you
> is that we would indeed need an escaping mechanism here.
True. I think a true shell globbing is overkill (so is regexp) and
just a simple wildcarding with '*' would be a good first step that
is easy to explain and later extend as needed.
Thanks.
^ permalink raw reply
* Re: show all merge conflicts
From: Jeff King @ 2017-01-27 17:51 UTC (permalink / raw)
To: Michael Spiegel; +Cc: git
In-Reply-To: <CANwu5-o=3p8QfWo9wQvok=UZESRVtF3Uxb3nEMZVv8AvkKYYGw@mail.gmail.com>
On Fri, Jan 27, 2017 at 11:56:08AM -0500, Michael Spiegel wrote:
> I'm trying to determine whether a merge required a conflict to resolve
> after the merge has occurred. The git book has some advice
> (https://git-scm.com/book/en/v2/Git-Tools-Advanced-Merging) to use
> `git show` on the merge commit or use `git log --cc -p -1`. These
> strategies work when the merge conflict was resolved with a change
> that is different from either parent. When the conflict is resolved
> with a change that is the same as one of the parents, then these
> commands are indistinguishable from a merge that did not conflict. Is
> it possible to distinguish between a conflict-free merge and a merge
> conflict that is resolved by with the changes from one the parents?
No. You'd have to replay the merge to know if it would have had
conflicts.
There was a patch series a few years ago that added a new diff-mode to
do exactly that, and show the diff against what was resolved. It had a
few issues (I don't remember exactly what) and never got merged.
Certainly one complication is that you don't know exactly _how_ the
merge was done in the first place (e.g., which merge strategy, which
custom merge drivers were in effect, etc). But in general, replaying
with a standard merge-recursive would get you most of what you want to
know.
I've done this manually sometimes when digging into erroneous merges
(e.g., somebody accidentally runs "git reset -- <paths>" in the middle
of a merge and throws away some changes.
You should be able to do:
git checkout $merge^1
git merge $merge^2
git diff -R $merge
to see what the original resolution did.
-Peff
^ permalink raw reply
* Re: [PATCH] fixup! worktree move: new command
From: Junio C Hamano @ 2017-01-27 17:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Nguyễn Thái Ngọc Duy
In-Reply-To: <alpine.DEB.2.20.1701271150200.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> always been discussed on the mailing list, I would like to kindly ask you
> to please add this patch to the nd/worktree-move branch for the time being
> (i.e. until Duy responds),
The tip of 'pu' (or anything beyond the tip of 'jch') is not always
expected to pass test or even build, and unless I know the OP is
leaky or usually slow, a "fixup!" that directly addresses the OP
tends to be left for the OP to pick up [*1*] to avoid "ah, you sent
a reroll but I already had one squashed locally", which is confusing
to both myself and OP (and adds burden to me).
It seems that OP is slower to respond than his usual, so I'll add it
to the tip of the topic so that it won't be lost.
[Footnote]
*1* It still is used on my end to leave a mental note to myself that
the topic is expected/waiting to be rerolled, but that is not
something you can read in "git log --first-parent master..pu".
^ permalink raw reply
* Re: [PATCH 11/12] receive-pack: treat namespace .have lines like alternates
From: Lukas Fleischer @ 2017-01-27 17:45 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <xmqqa8aec40a.fsf@gitster.mtv.corp.google.com>
On Wed, 25 Jan 2017 at 20:51:17, Junio C Hamano wrote:
> [...]
> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index 8f8762e4a..c55e2f993 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -251,8 +251,9 @@ static void show_ref(const char *path, const unsigned char *sha1)
> [...]
> > if (ref_is_hidden(path, path_full))
> [...]
> This is an unrelated tangent, but there may want to be a knob to
> make the code return here without even showing, to make the
> advertisement even smaller and also to stop miniscule information
> leakage? If the namespaced multiple projects are totally unrelated
> (i.e. "My sysadmin gave me a write access only to this single
> directory, so I am using the namespace feature to host these three
> projects that have nothing to do with each other"), showing objects
> of other namespaces will buy us nothing and the user is better off
> without this code showing these refs as ".have".
I think this is already possible using receive.hideRefs (which causes
the ref_is_hidden() branch above to return if applicable).
Having support for suppressing .have lines corresponding to different
namespaces was actually the reason I implemented 78a766ab6 (hideRefs:
add support for matching full refs, 2015-11-03). We have been using
namespaces for hosting the package Git repositories of the Arch Linux
User Repository [1] with a shared object storage for several months now.
See [2] for *some* technical details on how things are implemented; the
last section explains how the hideRefs mechanism can be used to limit
ref advertisement to the "active" namespace.
Regards,
Lukas
[1] https://aur.archlinux.org/
[2] https://git.archlinux.org/aurweb.git/plain/doc/git-interface.txt
^ permalink raw reply
* Re: [PATCH v2 1/1] reset: support the --stdin option
From: Jeff King @ 2017-01-27 17:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Jakub Narębski
In-Reply-To: <alpine.DEB.2.20.1701271832270.3469@virtualbox>
On Fri, Jan 27, 2017 at 06:34:46PM +0100, Johannes Schindelin wrote:
> > Is it worth clarifying that these are paths, not pathspecs? The word
> > "paths" is used to refer to the pathspecs on the command-line elsewhere
> > in the document.
> >
> > It might also be worth mentioning the quoting rules for the non-z case.
>
> I think this would be overkill. In reality, --stdin without -z does not
> make much sense, anyway.
I think with most Unix tools people tend to use the non-z forms until
they break, and then switch to the z form. :)
And in that sense, this transparently Just Works because the output will
often come from another git tool, which will quote as appropriate.
> If you feel strongly about it, I encourage you to submit a follow-up
> patch.
>
> The rest of your suggestions have been implemented in v3.
Thanks. I think the path/pathspec thing was the more important of the
two suggestions.
-Peff
^ permalink raw reply
* Re: [PATCH v3] t/Makefile: add a rule to re-run previously-failed tests
From: Johannes Schindelin @ 2017-01-27 17:21 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Sverre Rabbelier,
Ævar Arnfjörð Bjarmason, Matthieu Moy
In-Reply-To: <20170127170713.kn35br4xsdco7xth@sigill.intra.peff.net>
Hi Peff,
On Fri, 27 Jan 2017, Jeff King wrote:
> On Fri, Jan 27, 2017 at 03:17:36PM +0100, Johannes Schindelin wrote:
>
> > This patch automates the process of determinig which tests failed
> > previously and re-running them.
>
> s/determinig/determining/
Fixed in v4,
Johannes
^ permalink raw reply
* Re: [PATCH 2/2] use absolute_pathdup()
From: Junio C Hamano @ 2017-01-27 18:18 UTC (permalink / raw)
To: René Scharfe; +Cc: Johannes Schindelin, Git List
In-Reply-To: <0555c0aa-fcb4-19fb-a533-c451e1e477e3@web.de>
René Scharfe <l.s.r@web.de> writes:
> Hi Dscho,
>
> Am 27.01.2017 um 11:21 schrieb Johannes Schindelin:
>> On Thu, 26 Jan 2017, René Scharfe wrote:
>>> Apply the symantic patch for converting callers that duplicate the
>>
>> s/symantic/semantic/
>
> thank you! I wonder where this came from. And where my spellchecker
> went without as much as a farewell. Reinstalled it now..
>
> René
Thanks. I've already locally amended this.
^ permalink raw reply
* Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
From: Junio C Hamano @ 2017-01-27 18:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
In-Reply-To: <xmqqo9yt4o5i.fsf@gitster.mtv.corp.google.com>
Just to save us extra round-trip.
Junio C Hamano <gitster@pobox.com> writes:
>> +`GIT_SSH_VARIANT`::
>> + If this environment variable is set, it overrides the autodetection
>> + of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
>> + push' use. It can be set to either `ssh`, `plink`, `putty` or
>> + `tortoiseplink`. Any other value will be treated as normal ssh. This
>> + is useful in case that Git gets this wrong.
>
> Similarly this should mention GIT_SSH_COMMAND at least. It is crazy
> to set something that will cause misdetection to core.sshCommand and
> use this environment variable to fix it (instead of using ssh.variant),
> so I think it is OK not to mention core.sshCommand (but it would not
> hurt to do so).
>
>> diff --git a/connect.c b/connect.c
>> index 9f750eacb6..7b4437578b 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
>> return NULL;
>> }
>>
>> +static int handle_ssh_variant(int *port_option, int *needs_batch)
>> +{
>> ...
>> +}
> ...
>
> The string that came from the configuration must be freed, no? That
> is what I recall in Peff's comment yesterday.
The leak needs plugging in some way.
Because this thing is merely an escape hatch that somebody who needs
it needs to use it always (as opposed to one-shot basis), we do not
need to support the environment variable and go with only the
configuration, which may make it easier to plug the leak.
>> @@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char *url,
>> ssh_argv0 = xstrdup(ssh);
>> }
>>
>> - if (ssh_argv0) {
>> + if (!handle_ssh_variant(&port_option, &needs_batch) &&
>> + ssh_argv0) {
>
> I like the placement of this "if the user explicitly told us" much
> better.
> ...
> And that reasoning will lead to a realization "there is no reason to
> even do the split_cmdline() if the user explicitly told us". While
> that reasoning is correct and we _should_ further refactor, I didn't
> want to spend braincycles on that myself.
This _should_ above is a lot weaker than _must_.
IOW, I think it is acceptable to always split GIT_SSH_COMMAND into
tokens before we realize that the user used the escape hatch and the
splitting was a wasted effort. This is exactly because this thing
is an escape hatch that is expected to be rarely used. Of course,
if the "wasted effort" can be eliminated without sacrificing the
simplicity of the code, that is fine as well.
^ permalink raw reply
* Re: [PATCH 11/12] receive-pack: treat namespace .have lines like alternates
From: Jeff King @ 2017-01-27 17:58 UTC (permalink / raw)
To: Lukas Fleischer; +Cc: git, Junio C Hamano
In-Reply-To: <148553912610.7898.1319453517642036857@typhoon>
On Fri, Jan 27, 2017 at 06:45:26PM +0100, Lukas Fleischer wrote:
> > This is an unrelated tangent, but there may want to be a knob to
> > make the code return here without even showing, to make the
> > advertisement even smaller and also to stop miniscule information
> > leakage? If the namespaced multiple projects are totally unrelated
> > (i.e. "My sysadmin gave me a write access only to this single
> > directory, so I am using the namespace feature to host these three
> > projects that have nothing to do with each other"), showing objects
> > of other namespaces will buy us nothing and the user is better off
> > without this code showing these refs as ".have".
>
> I think this is already possible using receive.hideRefs (which causes
> the ref_is_hidden() branch above to return if applicable).
>
> Having support for suppressing .have lines corresponding to different
> namespaces was actually the reason I implemented 78a766ab6 (hideRefs:
> add support for matching full refs, 2015-11-03). We have been using
> namespaces for hosting the package Git repositories of the Arch Linux
> User Repository [1] with a shared object storage for several months now.
> See [2] for *some* technical details on how things are implemented; the
> last section explains how the hideRefs mechanism can be used to limit
> ref advertisement to the "active" namespace.
Thanks for the pointers. I think a "turn off namespace .have lines"
option would be easier for some cases, but what you've implemented is
much more flexible. So if people using namespaces are happy with it, I
don't see any need to add another way to do the same thing.
-Peff
^ permalink raw reply
* Re: [PATCH] mingw: allow hooks to be .exe files
From: Stefan Beller @ 2017-01-27 18:24 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Jeff King, git@vger.kernel.org
In-Reply-To: <alpine.DEB.2.20.1701271127520.3469@virtualbox>
On Fri, Jan 27, 2017 at 2:29 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Junio,
>
> On Thu, 26 Jan 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > On Wed, 25 Jan 2017, Jeff King wrote:
>> >
>> >> On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
>> >>
>> >> > - if (access(path.buf, X_OK) < 0)
>> >> > + if (access(path.buf, X_OK) < 0) {
>> >> > +#ifdef STRIP_EXTENSION
>> >> > + strbuf_addstr(&path, ".exe");
>> >>
>> >> I think STRIP_EXTENSION is a string. Should this line be:
>> >>
>> >> strbuf_addstr(&path, STRIP_EXTENSION);
>> >
>> > Yep.
>> >
>> > v2 coming,
>> > Johannes
>>
>> I think I've already tweaked it out when I queued the original one.
>
> After digging, I found your SQUASH commit. I had not known about that.
>
> In any case, I much rather prefer to have the final version of any patch
> or patch series I contribute to be identical between what you commit and
> what I sent to the mailing list. We do disagree from time to time, and I
> would like to have the opportunity of reviewing how you tweak my changes.
>
> Ciao,
> Johannes
I saw the queued up commit and that lead me to this thread here.
The commit message, while technically correct, seems a bit off. This is because
the commit message only talks about .exe extensions, but the change in code
doesn't even have the string "exe" mentioned once.
With this discussion here, it is easy for me to connect the dots, but
it would be nice
to have the full picture in the commit message. This is what I would write:
mingw: allow hooks to be .exe files
Executable files in Windows need to have the extension '.exe', otherwise
they do not work. Extend the hooks to not just look at the hard coded names,
but also at the extended names via the custom STRIP_EXTENSION, which
is defined as '.exe' in Windows.
Stefan
^ permalink raw reply
* Re: [PATCH v2 1/1] reset: support the --stdin option
From: Junio C Hamano @ 2017-01-27 18:30 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, Jakub Narębski
In-Reply-To: <20170127170422.lvpghp6jdud37zxx@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> A few minor suggestions:
>
>> +--stdin::
>> + Instead of taking list of paths from the command line,
>> + read list of paths from the standard input. Paths are
>> + separated by LF (i.e. one path per line) by default.
>> +
>> +-z::
>> + Only meaningful with `--stdin`; paths are separated with
>> + NUL character instead of LF.
>
> Is it worth clarifying that these are paths, not pathspecs? The word
> "paths" is used to refer to the pathspecs on the command-line elsewhere
> in the document.
If the code forces literal pathspecs, then what the user feeds to
the command is no longer pathspecs from the user's point of view,
and I agree that the distinction is important.
If the remainder of the documentation is loose in terminology and
the reason why we were able to get away with it was because we
consistently used "paths" when we meant "pathspec", these existing
mention of "paths" have to be tightened, either in this patch or a
preparatory step patch before this one, because the addition of
"this thing reads paths not pathspecs" is what makes them ambiguous.
Thanks. I re-read the part of the code that reads and unquotes as
necessary in the patch and they looked correct.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox