Re: [PATCH v4 3/9] t3422: new testcases for checking when incompatible options passed
On Tue, Jun 26, 2018 at 11:17 AM, Junio C Hamano wrote:
> Elijah Newren writes:
>
>> +# Rebase has lots of useful options like --whitepsace=fix, which are
>> +# actually all built in terms of flags to git-am. Since neither
>> +# --merge nor --interactive (nor any options that imply those two) use
>> +# git-am, using them together will result in flags like --whitespace=fix
>> +# being ignored. Make sure rebase warns the user and aborts instead.
>> +#
>> +
>> +test_rebase_am_only () {
>> + opt=$1
>> + shift
>> + test_expect_failure "$opt incompatible with --merge" "
>> + git checkout B^0 &&
>> + test_must_fail git rebase $opt --merge A
>> + "
>> +
>> + test_expect_failure "$opt incompatible with --strategy=ours" "
>> + git checkout B^0 &&
>> + test_must_fail git rebase $opt --strategy=ours A
>> + "
>> +
>> + test_expect_failure "$opt incompatible with --strategy-option=ours" "
>> + git checkout B^0 &&
>> + test_must_fail git rebase $opt --strategy=ours A
>
> This line is broken and it is carried over to later patches. It
> needs to be -Xours (or --strategy-option=ours, if we really want ot
> be verbose).
Indeed; I'll fix that up and resubmit. Thanks for catching it.
>> + "
>> +
>> + test_expect_failure "$opt incompatible with --interactive" "
>> + git checkout B^0 &&
>> + test_must_fail git rebase $opt --interactive A
>> + "
>> +
>> + test_expect_failure "$opt incompatible with --exec" "
>> + git checkout B^0 &&
>> + test_must_fail git rebase $opt --exec 'true' A
>> + "
>> +
>> +}
>
>> +
>> +test_rebase_am_only --whitespace=fix
>> +test_rebase_am_only --ignore-whitespace
>> +test_rebase_am_only --committer-date-is-author-date
>> +test_rebase_am_only -C4
>
> I was hesitant to hardcode what I perceive as limitations of non-am
> rebase implementations with a test like this, but once somebody
> fixes "rebase -i" for example to be capable of --whitespace=fix for
> example, then we can just drop one line from the above four (and
> write other tests for "rebase -i --whitespace=fix"). The
> test_rebase_am_only is to help us make sure what is (still) not
> supported by non-am rebases gets diagnosed as an error.
Yes, and I was thinking in particular that we could start by teaching
rebase -i to understand --ignore-space-change/--ignore-whitespace by
just transliterating it into -Xignore-space-change. That is, after
https://public-inbox.org/git/20180607050845.19779-2-new...@gmail.com/
is picked up and eventually merged down. Speaking of which, I need to
resubmit that.
> So my worry is totally unfounded, which is good.
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Wed, Jun 27, 2018 at 2:27 AM Johannes Sixt wrote:
> Am 27.06.2018 um 04:15 schrieb Elijah Newren:
> > On Tue, Jun 26, 2018 at 2:01 PM, Jeff King wrote:
> >> On Tue, Jun 26, 2018 at 04:46:18PM -0400, Eric Sunshine wrote:
> >>> Some of these dangers can be de-thoothed during the linting phase by
> >>> defining do-nothing shell functions:
> >>> cp () { :; }
> >>> That, at least, makes the scariest case ("rm") much less so.
> >>
> >> Now that's an interesting idea. We can't catch every dangerous action
> >> (notably ">" would be hard to override), but it should be pretty cheap
> >> to cover some obvious ones.
> >
> > Crazy idea: maybe we could defang it a little more thoroughly with
> > something like the following (apologies in advance if gmail whitespace
> > damages this):
> >
> > - if test "OK-117" != "$(test_eval_ "(exit 117) &&
> > $1${LF}${LF}echo OK-\$?" 3>&1)"
> > + if test "OK-117" != "$(test_eval_ "cd() { return 0; }
> > && PATH=/dev/null && export PATH && (exit 117) && $1${LF}${LF}echo
> > OK-\$?" 3>&1)"
Interesting idea (coupled with Hannes's point below)...
> I'd define all these functions as { return 1; } because we want to stop
> any && chain as early as possible (and with an exit code that is not the
> sentinel value).
A very sensible suggestion.
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Am 27.06.2018 um 04:15 schrieb Elijah Newren:
On Tue, Jun 26, 2018 at 2:01 PM, Jeff King wrote:
On Tue, Jun 26, 2018 at 04:46:18PM -0400, Eric Sunshine wrote:
Some of these dangers can be de-thoothed during the linting phase by
defining do-nothing shell functions:
cp () { :; }
mv () { :; }
ln () { :; }
That, at least, makes the scariest case ("rm") much less so.
Now that's an interesting idea. We can't catch every dangerous action
(notably ">" would be hard to override), but it should be pretty cheap
to cover some obvious ones.
-Peff
Crazy idea: maybe we could defang it a little more thoroughly with
something like the following (apologies in advance if gmail whitespace
damages this):
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 28315706be..7fda08a90a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -675,7 +675,7 @@ test_run_ () {
trace=
# 117 is magic because it is unlikely to match the exit
# code of other programs
- if test "OK-117" != "$(test_eval_ "(exit 117) &&
$1${LF}${LF}echo OK-\$?" 3>&1)"
+ if test "OK-117" != "$(test_eval_ "cd() { return 0; }
&& PATH=/dev/null && export PATH && (exit 117) && $1${LF}${LF}echo
OK-\$?" 3>&1)"
then
error "bug in the test script: broken &&-chain
or run-away HERE-DOC: $1"
fi
I'd define all these functions as { return 1; } because we want to stop
any && chain as early as possible (and with an exit code that is not the
sentinel value).
-- Hannes
[PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs
Replace `$(prefix)/etc/gitconfig` and `$(prefix)/etc/gitattributes` in
generated documentation with the paths chosen when building. Readers of
the documentation should not need to know how `$(prefix)` was defined.
It's also more consistent than sometimes using `$(prefix)/etc/gitconfig`
and other times using `/etc/gitconfig` to refer to the system-wide
config file.
Update the SYNOPSIS of gitattributes(5) to include the system-wide
config file as well.
Signed-off-by: Todd Zullinger
---
I noticed this while looking to update gitattributes.txt to
note the system-wide config file. I tested with and without
RUNTIME_PREFIX as well as make gitattributes.5 from within
Documentation. I believe all methods do the right thing.
I couldn't figure out a good way to have asciidoc expand the
attributes inside of a "`" literal, so I changed to the "+"
form. There might be a better way to do this, passing subs=
in asciidoc.conf, but I wasn't clear on where that would
fit. I tested with asciidoc and not asciidoctor.
Documentation/Makefile | 13 +
Documentation/config.txt| 4 ++--
Documentation/git-config.txt| 10 +-
Documentation/git.txt | 4 ++--
Documentation/gitattributes.txt | 4 ++--
5 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index d079d7c73a..75af671798 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -95,6 +95,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
prefix ?= $(HOME)
bindir ?= $(prefix)/bin
+sysconfdir ?= $(prefix)/etc
htmldir ?= $(prefix)/share/doc/git-doc
infodir ?= $(prefix)/share/info
pdfdir ?= $(prefix)/share/doc/git-doc
@@ -205,6 +206,18 @@ DEFAULT_EDITOR_SQ = $(subst ','\'',$(DEFAULT_EDITOR))
ASCIIDOC_EXTRA += -a 'git-default-editor=$(DEFAULT_EDITOR_SQ)'
endif
+ifndef ETC_GITCONFIG
+ETC_GITCONFIG = $(sysconfdir)/gitconfig
+endif
+ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
+ASCIIDOC_EXTRA += -a 'etc-gitconfig=$(ETC_GITCONFIG_SQ)'
+
+ifndef ETC_GITATTRIBUTES
+ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
+endif
+ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
+ASCIIDOC_EXTRA += -a 'etc-gitattributes=$(ETC_GITATTRIBUTES_SQ)'
+
QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir
QUIET_SUBDIR1 =
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1cc18a828c..ed903b60bd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -5,7 +5,7 @@ The Git configuration file contains a number of variables that
affect
the Git commands' behavior. The `.git/config` file in each repository
is used to store the configuration for that repository, and
`$HOME/.gitconfig` is used to store a per-user configuration as
-fallback values for the `.git/config` file. The file `/etc/gitconfig`
+fallback values for the `.git/config` file. The file +{etc-gitconfig}+
can be used to store a system-wide default configuration.
The configuration variables are used by both the Git plumbing
@@ -2815,7 +2815,7 @@ configuration files (e.g. `$HOME/.gitconfig`).
Example:
-/etc/gitconfig
+{etc-gitconfig}
push.pushoption = a
push.pushoption = b
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 18ddc78f42..0d5ea5b58e 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -114,10 +114,10 @@ See also <>.
--system::
For writing options: write to system-wide
- `$(prefix)/etc/gitconfig` rather than the repository
+ +{etc-gitconfig}+ rather than the repository
`.git/config`.
+
-For reading options: read only from system-wide `$(prefix)/etc/gitconfig`
+For reading options: read only from system-wide +{etc-gitconfig}+
rather than from all available files.
+
See also <>.
@@ -263,7 +263,7 @@ FILES
If not set explicitly with `--file`, there are four files where
'git config' will search for configuration options:
-$(prefix)/etc/gitconfig::
+{etc-gitconfig}::
System-wide configuration file.
$XDG_CONFIG_HOME/git/config::
@@ -310,11 +310,11 @@ ENVIRONMENT
GIT_CONFIG::
Take the configuration from the given file instead of .git/config.
Using the "--global" option forces this to ~/.gitconfig. Using the
- "--system" option forces this to $(prefix)/etc/gitconfig.
+ "--system" option forces this to {etc-gitconfig}.
GIT_CONFIG_NOSYSTEM::
Whether to skip reading settings from the system-wide
- $(prefix)/etc/gitconfig file. See linkgit:git[1] for details.
+ {etc-gitconfig} file. See linkgit:git[1] for details.
See also <>.
diff --git a/Documentation/git.txt b/Documentation/git.txt
index dba7f0c18e..6a4646f991 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -567,10 +567,10 @@ for further details.
`GIT_CONFIG_NOSYSTEM`::
Whether to skip reading settings from the system-wide
- `$(prefix)/etc/gitconfig` file. This environment var
[PATCH 1/2] gitignore.txt: clarify default core.excludesfile path
The default core.excludesfile path is $XDG_CONFIG_HOME/git/ignore. $HOME/.config/git/ignore is used if XDG_CONFIG_HOME is empty or unset, as described later in the document. Signed-off-by: Todd Zullinger --- Documentation/gitignore.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt index ff5d7f9ed6..d107daaffd 100644 --- a/Documentation/gitignore.txt +++ b/Documentation/gitignore.txt @@ -7,7 +7,7 @@ gitignore - Specifies intentionally untracked files to ignore SYNOPSIS -$HOME/.config/git/ignore, $GIT_DIR/info/exclude, .gitignore +$XDG_CONFIG_HOME/git/ignore, $GIT_DIR/info/exclude, .gitignore DESCRIPTION --- -- 2.18.0
[PATCH 2/2] dir.c: fix typos in core.excludesfile comment
Make it easier to find references to core.excludesfile and the default
$XDG_CONFIG_HOME/git/ignore path.
Signed-off-by: Todd Zullinger
---
I noticed the typo in core.excludesfile and $XDG_CONFIG_HOME while I was
verifing the previous change to clarify the documentation matched the code.
Fixing these minor issues in the comments will hopefully make it easier for
others to find the right places in the code in the future.
dir.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dir.c b/dir.c
index fe9bf58e4c..363a4837ae 100644
--- a/dir.c
+++ b/dir.c
@@ -2497,7 +2497,7 @@ void setup_standard_excludes(struct dir_struct *dir)
{
dir->exclude_per_dir = ".gitignore";
- /* core.excludefile defaulting to $XDG_HOME/git/ignore */
+ /* core.excludesfile defaulting to $XDG_CONFIG_HOME/git/ignore */
if (!excludes_file)
excludes_file = xdg_config_home("ignore");
if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
--
2.18.0
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 2:01 PM, Jeff King wrote:
> On Tue, Jun 26, 2018 at 04:46:18PM -0400, Eric Sunshine wrote:
>> Some of these dangers can be de-thoothed during the linting phase by
>> defining do-nothing shell functions:
>>
>> cp () { :; }
>> mv () { :; }
>> ln () { :; }
>>
>> That, at least, makes the scariest case ("rm") much less so.
>
> Now that's an interesting idea. We can't catch every dangerous action
> (notably ">" would be hard to override), but it should be pretty cheap
> to cover some obvious ones.
>
> -Peff
Crazy idea: maybe we could defang it a little more thoroughly with
something like the following (apologies in advance if gmail whitespace
damages this):
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 28315706be..7fda08a90a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -675,7 +675,7 @@ test_run_ () {
trace=
# 117 is magic because it is unlikely to match the exit
# code of other programs
- if test "OK-117" != "$(test_eval_ "(exit 117) &&
$1${LF}${LF}echo OK-\$?" 3>&1)"
+ if test "OK-117" != "$(test_eval_ "cd() { return 0; }
&& PATH=/dev/null && export PATH && (exit 117) && $1${LF}${LF}echo
OK-\$?" 3>&1)"
then
error "bug in the test script: broken &&-chain
or run-away HERE-DOC: $1"
fi
Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells
Jun 26, 2018 at 03:31:11PM -0700, Junio C Hamano wrote:
> Eric Sunshine writes:
>> On Tue, Jun 26, 2018 at 3:38 PM Junio C Hamano wrote:
>>> I like these earlier changes that fix existing breakage, of course.
>>> I also like many of the changes that simplify and/or modernise the
>>> test scripts very much, but they are unusable as-is as long as their
>>> justification is "chain-lint will start barfing on these constructs".
>>
>> Sorry, I'm having difficulty understanding.
>>
>> Are you saying that you don't want patches which exist merely to
>> pacify --chain-lint? (For instance, 2/29 "t0001: use "{...}" block
>> around "||" expression rather than subshell".)
>
> Yes.
>
>> Or are you saying that you don't like how the commit messages are
>> worded, and that they should instead emphasize that the change is good
>> for its own sake, without mentioning --chain-lint?
>
> Yes, too.
>
> For example, 03/29 is a good clean-up, and its value is not
> diminished even if we reject the subprocess munging --chain-lint in
> 29/29.
>
> As opposed to 02/29 which mostly is about appeasing the "shell
> parser" in 29/29 (or you could justify it saying "one less fork and
> process" if that gives us a measurable benefit).
This is a lighter-weight example of the practice described at
https://lkml.kernel.org/r/alpine.LFD.2.00.1001251002430.3574@localhost.localdomain/.
In my opinion it's good advice, often worth repeating.
Thanks,
Jonathan
Sie da....
Sie haben 5, OOO, OOO.OO EUR in das laufende Spendenprogramm der FIFA Fussball Weltmeisterschaft Russland 2018 gespendet. Bitte antworten Sie zurück für Ansprüche. --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
Re: [PATCH v2] filter-branch: skip commits present on --state-branch
Ian Campbell writes: > On Mon, 2018-06-25 at 21:07 -0700, Michael Barabanov wrote: >> The commits in state:filter.map have already been processed, so don't >> filter them again. This makes incremental git filter-branch much >> faster. >> >> Also add tests for --state-branch option. >> >> Signed-off-by: Michael Barabanov > > Acked-by: Ian Campbell Thanks. > >> --- >> git-filter-branch.sh | 1 + >> t/t7003-filter-branch.sh | 15 +++ >> 2 files changed, 16 insertions(+) >> >> diff --git a/git-filter-branch.sh b/git-filter-branch.sh >> index ccceaf19a..5c5afa2b9 100755 >> --- a/git-filter-branch.sh >> +++ b/git-filter-branch.sh >> @@ -372,6 +372,7 @@ while read commit parents; do >> git_filter_branch__commit_count=$(($git_filter_branch__commi >> t_count+1)) >> >> report_progress >> +test -f "$workdir"/../map/$commit && continue >> >> case "$filter_subdir" in >> "") >> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh >> index ec4b160dd..e23de7d0b 100755 >> --- a/t/t7003-filter-branch.sh >> +++ b/t/t7003-filter-branch.sh >> @@ -107,6 +107,21 @@ test_expect_success 'test that the directory was >> renamed' ' >> test dir/D = "$(cat diroh/D.t)" >> ' >> >> +V=$(git rev-parse HEAD) >> + >> +test_expect_success 'populate --state-branch' ' >> +git filter-branch --state-branch state -f --tree-filter >> "touch file || :" HEAD >> +' >> + >> +W=$(git rev-parse HEAD) >> + >> +test_expect_success 'using --state-branch to skip already rewritten >> commits' ' >> +test_when_finished git reset --hard $V && >> +git reset --hard $V && >> +git filter-branch --state-branch state -f --tree-filter >> "touch file || :" HEAD && >> +test_cmp_rev $W HEAD >> +' >> + >> git tag oldD HEAD~4 >> test_expect_success 'rewrite one branch, keeping a side branch' ' >> git branch modD oldD &&
Re: [PATCH v6 4/4] stash: convert pop to builtin
Hi Paul,
I think I had revewied these 4 patches before, and I'd wager a bet that
you addressed all of my suggestions, if any.
I had a look over patches 2-4, and want to take a little bit more time
tomorrow to pour over patch 1 (which is a little larger, as it lays a lot
of ground work), to make sure that I cannot find anything else to improve.
Well done so far!
Dscho
On Mon, 25 Jun 2018, Paul-Sebastian Ungureanu wrote:
> From: Joel Teichroeb
>
> Add stash pop to the helper and delete the pop_stash, drop_stash,
> assert_stash_ref functions from the shell script now that they
> are no longer needed.
>
> Signed-off-by: Joel Teichroeb
> Signed-off-by: Paul-Sebastian Ungureanu
> ---
> builtin/stash--helper.c | 36 ++-
> git-stash.sh| 47 ++---
> 2 files changed, 37 insertions(+), 46 deletions(-)
>
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index fbf78249c..a38d6ae8a 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -13,7 +13,7 @@
>
> static const char * const git_stash_helper_usage[] = {
> N_("git stash--helper drop [-q|--quiet] []"),
> - N_("git stash--helper apply [--index] [-q|--quiet] []"),
> + N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
> []"),
> N_("git stash--helper branch []"),
> N_("git stash--helper clear"),
> NULL
> @@ -24,6 +24,11 @@ static const char * const git_stash_helper_drop_usage[] = {
> NULL
> };
>
> +static const char * const git_stash_helper_pop_usage[] = {
> + N_("git stash--helper pop [--index] [-q|--quiet] []"),
> + NULL
> +};
> +
> static const char * const git_stash_helper_apply_usage[] = {
> N_("git stash--helper apply [--index] [-q|--quiet] []"),
> NULL
> @@ -528,6 +533,33 @@ static int drop_stash(int argc, const char **argv, const
> char *prefix)
> return ret;
> }
>
> +static int pop_stash(int argc, const char **argv, const char *prefix)
> +{
> + int index = 0, ret;
> + struct stash_info info;
> + struct option options[] = {
> + OPT__QUIET(&quiet, N_("be quiet, only report errors")),
> + OPT_BOOL(0, "index", &index,
> + N_("attempt to recreate the index")),
> + OPT_END()
> + };
> +
> + argc = parse_options(argc, argv, prefix, options,
> + git_stash_helper_pop_usage, 0);
> +
> + if (get_stash_info(&info, argc, argv))
> + return -1;
> +
> + assert_stash_ref(&info);
> + if ((ret = do_apply_stash(prefix, &info, index)))
> + printf_ln(_("The stash entry is kept in case you need it
> again."));
> + else
> + ret = do_drop_stash(prefix, &info);
> +
> + free_stash_info(&info);
> + return ret;
> +}
> +
> static int branch_stash(int argc, const char **argv, const char *prefix)
> {
> const char *branch = NULL;
> @@ -589,6 +621,8 @@ int cmd_stash__helper(int argc, const char **argv, const
> char *prefix)
> return !!clear_stash(argc, argv, prefix);
> else if (!strcmp(argv[0], "drop"))
> return !!drop_stash(argc, argv, prefix);
> + else if (!strcmp(argv[0], "pop"))
> + return !!pop_stash(argc, argv, prefix);
> else if (!strcmp(argv[0], "branch"))
> return !!branch_stash(argc, argv, prefix);
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 29d9f4425..8f2640fe9 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -554,50 +554,6 @@ assert_stash_like() {
> }
> }
>
> -is_stash_ref() {
> - is_stash_like "$@" && test -n "$IS_STASH_REF"
> -}
> -
> -assert_stash_ref() {
> - is_stash_ref "$@" || {
> - args="$*"
> - die "$(eval_gettext "'\$args' is not a stash reference")"
> - }
> -}
> -
> -apply_stash () {
> - cd "$START_DIR"
> - git stash--helper apply "$@"
> - res=$?
> - cd_to_toplevel
> - return $res
> -}
> -
> -pop_stash() {
> - assert_stash_ref "$@"
> -
> - if apply_stash "$@"
> - then
> - drop_stash "$@"
> - else
> - status=$?
> - say "$(gettext "The stash entry is kept in case you need it
> again.")"
> - exit $status
> - fi
> -}
> -
> -drop_stash () {
> - assert_stash_ref "$@"
> -
> - git reflog delete --updateref --rewrite "${REV}" &&
> - say "$(eval_gettext "Dropped \${REV} (\$s)")" ||
> - die "$(eval_gettext "\${REV}: Could not drop stash entry")"
> -
> - # clear_stash if we just dropped the last stash entry
> - git rev-parse --verify --quiet "$ref_stash@{0}" >/dev/null ||
> - clear_stash
> -}
> -
> test "$1" = "-p" && set "push" "$@"
>
> PARSE_CACHE='--not-parsed'
> @@ -655,7 +611,8 @@ drop)
> ;;
> pop)
> shift
> - pop_stash "$@"
> + cd "$START_DIR"
> + git stash--helper pop "$@"
> ;;
> branch)
> shift
> --
Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells
Eric Sunshine writes:
> On Tue, Jun 26, 2018 at 3:38 PM Junio C Hamano wrote:
>> I first looked at 29/29 and got heavily inclined to reject that
>> step, and then continued reading from 1/29 to around 15/29.
>>
>> I like these earlier changes that fix existing breakage, of course.
>> I also like many of the changes that simplify and/or modernise the
>> test scripts very much, but they are unusable as-is as long as their
>> justification is "chain-lint will start barfing on these constructs".
>
> Sorry, I'm having difficulty understanding.
>
> Are you saying that you don't want patches which exist merely to
> pacify --chain-lint? (For instance, 2/29 "t0001: use "{...}" block
> around "||" expression rather than subshell".)
Yes.
> Or are you saying that you don't like how the commit messages are
> worded, and that they should instead emphasize that the change is good
> for its own sake, without mentioning --chain-lint?
Yes, too.
For example, 03/29 is a good clean-up, and its value is not
diminished even if we reject the subprocess munging --chain-lint in
29/29.
As opposed to 02/29 which mostly is about appeasing the "shell
parser" in 29/29 (or you could justify it saying "one less fork and
process" if that gives us a measurable benefit).
Re: [PATCH v6 3/4] stash: convert branch to builtin
Hi Paul,
On Mon, 25 Jun 2018, Paul-Sebastian Ungureanu wrote:
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 84a537f39..fbf78249c 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -522,6 +528,41 @@ static int drop_stash(int argc, const char **argv, const
> char *prefix)
> return ret;
> }
>
> +static int branch_stash(int argc, const char **argv, const char *prefix)
> +{
> + const char *branch = NULL;
> + int ret;
> + struct argv_array args = ARGV_ARRAY_INIT;
> + struct stash_info info;
> + struct option options[] = {
> + OPT_END()
> + };
> +
> + argc = parse_options(argc, argv, prefix, options,
> + git_stash_helper_branch_usage, 0);
> +
> + if (argc == 0)
> + return error(_("No branch name specified"));
> +
> + branch = argv[0];
> +
> + if (get_stash_info(&info, argc - 1, argv + 1))
> + return -1;
> +
> + argv_array_pushl(&args, "checkout", "-b", NULL);
> + argv_array_push(&args, branch);
> + argv_array_push(&args, oid_to_hex(&info.b_commit));
Why not combine these? _pushl() takes a NULL-terminated list:
argv_array_pushl(&args, "checkout", "-b", branch,
oid_to_hex(&info.b_commit), NULL);
> + ret = cmd_checkout(args.argc, args.argv, prefix);
I guess it is okay to run that, but the cmd_() functions are not *really*
meant to be called this way... Personally, I would be more comfortable
with a `run_command()` here, i.e. with a spawned process, until the time
when checkout.c/checkout.h have learned enough API for a direct call.
Ciao,
Dscho
Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect
Christian Couder writes: > Obviousness is often not the same for everybody. ... which you just learned---what you thought obvious turns out to be not so obvious after all, so you adjust to help your readers. >> In this particular case it even feels as if this test is not even testing >> what it should test at all: >> >> - it should verify that all of the commits in the first parent lineage are >> part of the list > > It does that. > >> - it should verify that none of the other commits are in the list > > It does that too. But the point is it does a lot more by insisting exact output. For example, the version I reviewed had a two "expected output", and said that the actual output must match either one of them. I guess it was because there were two entries with the same distance and we cannot rely on which order they appear in the result? If a test history gained another entry with the same distance, then would we need 6 possible expected output because we cannot rely on the order in which these three come out? That was the only thing I was complaining about. Dscho gave me too much credit and read a lot more good things than what I actually meant to say ;-). >> And that is really all there is to test. Another is that "rev-list --bisect-all" promises that the entries are ordered by the distance value. So taking the above three points, perhaps cat >expectactual.sorted && sort expect >expect.sorted && test_cmp expect.sorted actual.sorted # Make sure the entries are sorted in the dist order sed -e 's/.*(dist=\([1-9]*[0-9]\)).*/\1/' actual >actual.dists && sort actual.dists >actual.dists.sorted && test_cmp actual.dists.sorted actual.dists is what I would have expected.
Herzlichen Glückwunsch, Sie haben 650 000 €
Herzlichen Glückwunsch, Sie haben in den Euro Millions / Google 650.000 Euro gewonnen Promo monatliche Auslosungen am 1. Juni 2018 statt. Kontaktieren Sie unseren Schadenmakler mit den folgenden Informationen für Schäden auf dieser E-Mail: johnlubos...@gmail.com 1. Vollständiger Name: 2. Adresse: 3. Geschlecht: 4. Alter: 5. Beruf: 6. Telefon: Robert Avtandiltayn Online-Koordinator
Re: [PATCH v6 2/4] stash: convert drop and clear to builtin
Hi Paul,
On Mon, 25 Jun 2018, Paul-Sebastian Ungureanu wrote:
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 1c4387b10..84a537f39 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -414,6 +451,77 @@ static int apply_stash(int argc, const char **argv,
> const char *prefix)
> return ret;
> }
>
> +static int do_drop_stash(const char *prefix, struct stash_info *info)
> +{
> + struct argv_array args = ARGV_ARRAY_INIT;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + int ret;
> +
> + /*
> + * reflog does not provide a simple function for deleting refs. One will
> + * need to be added to avoid implementing too much reflog code here
> + */
> + argv_array_pushl(&args, "reflog", "delete", "--updateref", "--rewrite",
> + NULL);
> + argv_array_push(&args, info->revision.buf);
> + ret = cmd_reflog(args.argc, args.argv, prefix);
> + if (!ret) {
> + if (!quiet)
> + printf(_("Dropped %s (%s)\n"), info->revision.buf,
> +oid_to_hex(&info->w_commit));
> + } else {
> + return error(_("%s: Could not drop stash entry"),
> info->revision.buf);
> + }
> +
> + /*
> + * This could easily be replaced by get_oid, but currently it will
> throw a
> + * fatal error when a reflog is empty, which we can not recover from
> + */
> + cp.git_cmd = 1;
> + /* Even though --quiet is specified, rev-parse still outputs the hash */
> + cp.no_stdout = 1;
> + argv_array_pushl(&cp.args, "rev-parse", "--verify", "--quiet", NULL);
> + argv_array_pushf(&cp.args, "%s@{0}", ref_stash);
> + ret = run_command(&cp);
I thought you had introduced `get_oidf()` specifically so you could avoid
the `rev-parse` call... `get_oidf(&dummy_oid, "%s@{0}", ref_stash)` should
do this, right?
Ciao,
Dscho
Re: [PATCH v6 0/4] stash: Convert some `git stash` commands to a builtin
Hi Paul, On Mon, 25 Jun 2018, Paul-Sebastian Ungureanu wrote: > This second series of patches contains commits to convert `apply`, `drop`, > `clear`, `branch`, `pop` stash subcommands to builtins. > > > Joel Teichroeb (4): > stash: convert apply to builtin > stash: convert drop and clear to builtin > stash: convert branch to builtin > stash: convert pop to builtin Were there any changes since v5 in these patches? Ciao, Dscho
Re: [PATCH v6 4/4] stash: renamed test cases to be more descriptive
Hi Paul, On Mon, 25 Jun 2018, Paul-Sebastian Ungureanu wrote: > Renamed some test cases' labels to be more descriptive and under 80 > characters per line. > > Signed-off-by: Paul-Sebastian Ungureanu As I suggested this kind of change, I am obviously happy with this patch. Apart from minor suggestions, I think this patch series is good to go. Thank you! Dscho
Re: [PATCH v6 3/4] stash: update test cases conform to coding guidelines
Hi Paul, On Mon, 25 Jun 2018, Paul-Sebastian Ungureanu wrote: > Removed whitespaces after redirection operators. That is accurate a description of what the patch does. Let's add the why (and change the tense to present tense, as is the custom in our commit messages): This adjusts t3903-stash.sh to conform with our coding conventions better, by removing whitespace between redirection operators and their file names. The patch does two more things, though: it breaks at least one long line, and it renames at least two non-descriptive files to the much better name `expected`. Maybe you want to mention that in the commit message, to? Thanks, Dscho
Re: [PATCH v6 1/4] sha1-name.c: added 'get_oidf', which acts like 'get_oid'
Hi Paul, as a general rule, we try to keep the commit subjects in the imperative, i.e. sha1-name.c: add 'get_oidf', which acts like 'get_oid' On Mon, 25 Jun 2018, Paul-Sebastian Ungureanu wrote: > Compared to 'get_oid', 'get_oidf' has as parameters a > printf format string and the additional arguments. This > will help simplify the code in subsequent commits. Good. The patch is obviously correct, from my perspective. Ciao, Dscho
Re: [PATCH v6 0/4] stash: add new tests and introduce a new helper function
Hi, On Mon, 25 Jun 2018, Paul-Sebastian Ungureanu wrote: > This first series of patches does bring some changes and improvements to > the test suite. One of the patches also introduces a new function > `get_oidf()` which will be hepful for the incoming patches related to > `git stash`. For reviewers: it is *my* fault that this patch submission is a bit funny with two 1/4 and one 1/6 patches... *I* suggested to not send a 14-strong patch series but split it into three, and then I failed to explain the correct invocation to do that from the command-line. My sincere apologies, Dscho
Re: [PATCH] rebase -i: Fix white space in comments
On 26 Jun 2018, at 16:44, Johannes Schindelin wrote: >There is of course one other way to fix this, and that is by rewriting >this in C. > >Which Alban has done here ;-) > >http://public-inbox.org/git/20180626161643.31152-3-alban.gr...@gmail.com Oh, i'm sorry, i didn't see that. That change does appear to solve the same problem, so i'm happy to defer to it. Thanks for looking! dana
Re: [GSoC][PATCH 1/1] sequencer: print an error message if append_todo_help() fails
Hi Junio, On Tue, 26 Jun 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > On Tue, 26 Jun 2018, Alban Gruin wrote: > > > >> This adds an error when append_todo_help() fails to write its message to > >> the todo file. > >> > >> Signed-off-by: Alban Gruin > > > > ACK. > > > > We *may* want to fold that into the commit that adds `append_todo_help()`. > > Absolutely. This looks more like an "oops, I made a mess and here > is a fix on top", and even worse, it does not make an effort to help > readers where the mess was made (iow, which commit it goes on to > of); it is better to be squashed in. > > I do not know offhand who Alban's mentors are, but one thing I think > is a good thing for them to teach is how to better organize the > changes with readers in mind. The author of a patch series knows > his or her patches and how they relate to each other a lot better > than the readers of patches, who are reading not just his or her > patches but the ones from a lot wider set of contributors. Even > though append-todo-help and edit-todo may have been developed as > separate steps in author's mind, it is criminal to send them as if > they are completely separate topics that can independently applied, > especially when one depends on the other. It is a lot more helpful > to the readers if they were sent as a larger single series, because > doing so _will_ tell the readers which order the dependency goes. Chris & Stefan are Alban's mentors, and I spend quite a bit of my time on IRC, ready to help Alban when he has questions. Chris & Stephan mainly act as first-line reviewers. > > And, as I mentioned previously, I would love for that function to be > > used as an excuse to introduce the long-overdue `interactive-rebase.c` > > I am not sure if I like this direction. Blame me, not Alban. I am pretty familiar with sequencer.c, and I know that it is way too large. > As newbies are often very bad at coming up with APIs and naming global > functions, keeping everything as "static" inside a single sequencer.c > tends to avoid contaminating the global namespace. Then I just need to make sure to suggest good names that are safe for the global namespace, don't I? Seeing as sequencer.c is so long already, it is own little mega namespace anyway, so we already have to be very careful *within* sequencer.c. Ciao, Dscho
[PATCH v3] Documentation: declare "core.ignorecase" as internal variable
The current description of "core.ignoreCase" reads like an option which is intended to be changed by the user while it's actually expected to be set by Git on initialization only. Subsequently, Git relies on the proper configuration of this variable, as noted by Bryan Turner [1]: Git on a case-insensitive filesystem (APFS, HFS+, FAT32, exFAT, vFAT, NTFS, etc.) is not designed to be run with anything other than core.ignoreCase=true. [1] https://marc.info/?l=git&m=152998665813997&w=2 mid:cagyf7-gee8jrgpkme9rhkptheq6p1+ebpmmwatmh01uo3bf...@mail.gmail.com Signed-off-by: Marc Strapetz --- Documentation/config.txt | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 1cc18a828..c70cfe956 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -390,16 +390,19 @@ core.hideDotFiles:: default mode is 'dotGitOnly'. core.ignoreCase:: - If true, this option enables various workarounds to enable + Internal variable which enables various workarounds to enable Git to work better on filesystems that are not case sensitive, - like FAT. For example, if a directory listing finds - "makefile" when Git expects "Makefile", Git will assume + like APFS, HFS+, FAT, NTFS, etc. For example, if a directory listing + finds "makefile" when Git expects "Makefile", Git will assume it is really the same file, and continue to remember it as "Makefile". + The default is false, except linkgit:git-clone[1] or linkgit:git-init[1] will probe and set core.ignoreCase true if appropriate when the repository is created. ++ +Git relies on the proper configuration of this variable for your operating +and file system. Modifying this value may result in unexpected behavior. core.precomposeUnicode:: This option is only used by Mac OS implementation of Git. -- 2.17.0.rc0.3.gb1b5a51b2
Re: [GSoC][PATCH v3 2/2] rebase-interactive: rewrite the edit-todo functionality in C
Hi Alban, On Tue, 26 Jun 2018, Alban Gruin wrote: > This rewrites the edit-todo functionality from shell to C. > > To achieve that, a new command mode, `edit-todo`, is added, and the > `write-edit-todo` flag is removed, as the shell script does not need to > write the edit todo help message to the todo list anymore. > > The shell version is then stripped in favour of a call to the helper. > > Signed-off-by: Alban Gruin Both patches are ACKed by me, Dscho
Re: [GSoC][PATCH v3 0/2] rebase -i: rewrite the edit-todo functionality in C
Hi Alban, On Tue, 26 Jun 2018, Alban Gruin wrote: > This patch rewrites the edit-todo functionality from shell to C. This is > part of the effort to rewrite interactive rebase in C. > > This patch is based on the fourth iteration of my series rewriting > append_todo_help() in C. > > Changes since v2: > > - Moving edit_todo() from sequencer.c to interactive-rebase.c. Excellent. Thank you, Dscho
Re: [PATCH] rebase -i: Fix white space in comments
Hi, me again, On Tue, 26 Jun 2018, Johannes Schindelin wrote: > On Tue, 26 Jun 2018, Johannes Schindelin wrote: > > > On Tue, 26 Jun 2018, dana wrote: > > > > > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh > > > index 299ded213..a31af6d4c 100644 > > > --- a/git-rebase--interactive.sh > > > +++ b/git-rebase--interactive.sh > > > @@ -222,9 +222,9 @@ $comment_char $(eval_ngettext \ > > > EOF > > > append_todo_help > > > gettext " > > > - However, if you remove everything, the rebase will be aborted. > > > +However, if you remove everything, the rebase will be aborted. > > > > > > - " | git stripspace --comment-lines >>"$todo" > > > +" | git stripspace --comment-lines >>"$todo" > > > > > > if test -z "$keep_empty" > > > then > > This does the job, and I am fine with this way of doing things, and there > seems to be a lot of precedent doing it this way e.g. in git-bisect.sh. There is of course one other way to fix this, and that is by rewriting this in C. Which Alban has done here ;-) http://public-inbox.org/git/20180626161643.31152-3-alban.gr...@gmail.com Ciao, Johannes
Re: [GSoC][PATCH v4 2/2] rebase--interactive: rewrite append_todo_help() in C
Hi Alban, On Tue, 26 Jun 2018, Alban Gruin wrote: > This rewrites append_todo_help() from shell to C. It also incorporates > some parts of initiate_action() and complete_action() that also write > help texts to the todo file. > > This also introduces the source file rebase-interactive.c. This file > will contain functions necessary for interactive rebase that are too > specific for the sequencer, and is part of libgit.a. > > Two flags are added to rebase--helper.c: one to call > append_todo_help() (`--append-todo-help`), and another one to tell > append_todo_help() to write the help text suited for the edit-todo > mode (`--write-edit-todo`). > > Finally, append_todo_help() is removed from git-rebase--interactive.sh > to use `rebase--helper --append-todo-help` instead. Looks good! Thanks, Dscho
Re: [PATCH v5 7/8] fetch-pack: put shallow info in output parameter
Brandon Williams writes: > Expand the transport fetch method signature, by adding an output > parameter, to allow transports to return information about the refs they > have fetched. Then communicate shallow status information through this > mechanism instead of by modifying the input list of refs. Makes sense. Would this mechanism also allow us to be more explicit about the "tag following"?
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 5:33 PM Elijah Newren wrote: > On Tue, Jun 26, 2018 at 1:22 PM, Jeff King wrote: > > Another option is to not enable this slightly-more-dangerous linting by > > default. But that would probably rob it of its usefulness, since it > > would just fall to some brave soul to later crank up the linting and fix > > everybody else's mistakes. > > This may be a dumb question, but why can't we run under errexit? If > we could do that, we wouldn't need the &&-chaining, and bash would > parse the shell for us and exit whenever one command failed. (Is the > reason for this documented somewhere? I couldn't find it...) I'm not sure if it's documented anywhere, but it has been discussed. In particular, see [1], especially [2], and [3]. Peff summed up by saying: So I dunno. I think "set -e" is kind of a dangerous lure. It works so well _most_ of the time that you start to rely on it, but it really does have some funny corner cases (even on modern shells, and for all I know, the behavior above is mandated by POSIX). [1]: https://public-inbox.org/git/xmqq384zha6s@gitster.dls.corp.google.com/ [2]: https://public-inbox.org/git/20150320172406.ga15...@peff.net/ [3]: https://public-inbox.org/git/xmqqoannfu84@gitster.dls.corp.google.com/
Re: [GSoC][PATCH v4 1/2] sequencer: make two functions and an enum from sequencer.c public
Hi Alban,
On Tue, 26 Jun 2018, Alban Gruin wrote:
> diff --git a/sequencer.h b/sequencer.h
> index c5787c6b5..08397b0d1 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -3,6 +3,7 @@
>
> const char *git_path_commit_editmsg(void);
> const char *git_path_seq_dir(void);
> +const char *rebase_path_todo(void);
>
> #define APPEND_SIGNOFF_DEDUP (1u << 0)
>
> @@ -57,6 +58,10 @@ struct replay_opts {
> };
> #define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
>
> +enum check_level {
> + CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
> +};
> +
While this is contained within scopes that include `sequencer.h`, it *is*
public now, so I am slightly uneasy about keeping this enum so generic.
Maybe we want to use
enum missing_commit_check_level {
MISSING_COMMIT_CHECK_IGNORE = 0,
MISSING_COMMIT_CHECK_WARN,
MISSING_COMMIT_CHECK_ERROR
};
instead?
Ciao,
Dscho
Re: [PATCH v5 6/8] fetch: refactor to make function args narrower
Brandon Williams writes: > Refactor find_non_local_tags and get_ref_map to only take the > information they need instead of the entire transport struct. Besides > improving code clarity, this also improves their flexibility, allowing > for a different set of refs to be used instead of relying on the ones > stored in the transport struct. Makes sense. One can argue that the original has less risk of set of refs used for calls to this function vs calls for others go out of sync, but the objective of this series is to allow them to be different ;-) I am not sure if I understand/agree with the split of parameters done to get_ref_map() at this step, but hopefully its benefit would become obvious once I read later steps.
Re: [GSoC][PATCH v4 0/2] rebase -i: rewrite append_todo_help() in C
Hi Alban, On Tue, 26 Jun 2018, Alban Gruin wrote: > This patch rewrites append_todo_help() from shell to C. The C version > covers a bit more than the old shell version. To achieve that, some > parameters were added to rebase--helper. > > This also introduce a new source file, rebase-interactive.c. > > This is part of the effort to rewrite interactive rebase in C. > > This is based on next, as of 2018-06-26. Out of curiosity: which commits that are not yet in `master` are required? > Changes since v3: > > - Show an error message when append_todo_help() fails to edit the todo >list. > > - Introducing rebase-interactive.c to contain functions necessary for >interactive rebase. Thank you very much! Dscho
Re: [PATCH] rebase -i: Fix white space in comments
Hi, and now for the review... On Tue, 26 Jun 2018, Johannes Schindelin wrote: > On Tue, 26 Jun 2018, dana wrote: > > > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh > > index 299ded213..a31af6d4c 100644 > > --- a/git-rebase--interactive.sh > > +++ b/git-rebase--interactive.sh > > @@ -222,9 +222,9 @@ $comment_char $(eval_ngettext \ > > EOF > > append_todo_help > > gettext " > > - However, if you remove everything, the rebase will be aborted. > > +However, if you remove everything, the rebase will be aborted. > > > > - " | git stripspace --comment-lines >>"$todo" > > +" | git stripspace --comment-lines >>"$todo" > > > > if test -z "$keep_empty" > > then This does the job, and I am fine with this way of doing things, and there seems to be a lot of precedent doing it this way e.g. in git-bisect.sh. If my ACK is welcome, you hereby have it. Ciao, Johannes
Re: [PATCH v5 3/8] upload-pack: test negotiation with changing repository
Brandon Williams writes:
> diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
> new file mode 100644
> index 0..8a9a5aca0
> --- /dev/null
> +++ b/t/lib-httpd/one-time-sed.sh
> @@ -0,0 +1,22 @@
> +#!/bin/sh
> +
> +# If "one-time-sed" exists in $HTTPD_ROOT_PATH, run sed on the HTTP response,
> +# using the contents of "one-time-sed" as the sed command to be run. If the
> +# response was modified as a result, delete "one-time-sed" so that subsequent
> +# HTTP responses are no longer modified.
;-) clever.
> +#
> +# This can be used to simulate the effects of the repository changing in
> +# between HTTP request-response pairs.
> +if [ -e one-time-sed ]; then "$GIT_EXEC_PATH/git-http-backend" >out
Style (cf. Documentation/CodingGuidelines).
> +
> + sed "$(cat one-time-sed)" out_modified
> +
> + if diff out out_modified >/dev/null; then
> + cat out
> + else
> + cat out_modified
> + rm one-time-sed
> + fi
> +else
> + "$GIT_EXEC_PATH/git-http-backend"
> +fi
OK. I was worried if the removal-after-use was about _this_ script
(in which case it is a bad hygiene), but this is not a one-time
script, but merely the mechanism to use the one-time sed script.
Perhaps rename this to t/lib-httpd/apply-one-time-sed.sh or something
to avoid confusion?
> diff --git a/t/t5703-upload-pack-ref-in-want.sh
> b/t/t5703-upload-pack-ref-in-want.sh
> index 0ef182970..a4fe0e7e4 100755
> --- a/t/t5703-upload-pack-ref-in-want.sh
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -150,4 +150,72 @@ test_expect_success 'want-ref with ref we already have
> commit for' '
> check_output
> '
>
> +. "$TEST_DIRECTORY"/lib-httpd.sh
> +start_httpd
> +
> +REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
> +LOCAL_PRISTINE="$(pwd)/local_pristine"
> +
> +test_expect_success 'setup repos for change-while-negotiating test' '
> + (
> + git init "$REPO" &&
> + cd "$REPO" &&
> + >.git/git-daemon-export-ok &&
> + test_commit m1 &&
> + git tag -d m1 &&
> +
> + # Local repo with many commits (so that negotiation will take
> + # more than 1 request/response pair)
> + git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo";
> "$LOCAL_PRISTINE" &&
> + cd "$LOCAL_PRISTINE" &&
> + git checkout -b side &&
> + for i in $(seq 1 33); do test_commit s$i; done &&
> +
> + # Add novel commits to upstream
> + git checkout master &&
> + cd "$REPO" &&
> + test_commit m2 &&
> + test_commit m3 &&
> + git tag -d m2 m3
> + ) &&
> + git -C "$LOCAL_PRISTINE" remote set-url origin
> "http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo"; &&
> + git -C "$LOCAL_PRISTINE" config protocol.version 2
> +'
> +
> +inconsistency() {
Style. "inconsistency () {"
> + # Simulate that the server initially reports $2 as the ref
> + # corresponding to $1, and after that, $1 as the ref corresponding to
> + # $1. This corresponds to the real-life situation where the server's
> + # repository appears to change during negotiation, for example, when
> + # different servers in a load-balancing arrangement serve (stateless)
> + # RPCs during a single negotiation.
> + printf "s/%s/%s/" \
> +$(git -C "$REPO" rev-parse $1 | tr -d "\n") \
> +$(git -C "$REPO" rev-parse $2 | tr -d "\n") \
> +>"$HTTPD_ROOT_PATH/one-time-sed"
> +}
> +
> +test_expect_success 'server is initially ahead - no ref in want' '
> + git -C "$REPO" config uploadpack.allowRefInWant false &&
> + rm -rf local &&
> + cp -r "$LOCAL_PRISTINE" local &&
> + inconsistency master 1234567890123456789012345678901234567890 &&
> + test_must_fail git -C local fetch 2>err &&
> + grep "ERR upload-pack: not our ref" err
> +'
> +
> +test_expect_success 'server is initially behind - no ref in want' '
> + git -C "$REPO" config uploadpack.allowRefInWant false &&
> + rm -rf local &&
> + cp -r "$LOCAL_PRISTINE" local &&
> + inconsistency master "master^" &&
> + git -C local fetch &&
> +
> + git -C "$REPO" rev-parse --verify "master^" >expected &&
> + git -C local rev-parse --verify refs/remotes/origin/master >actual &&
> + test_cmp expected actual
> +'
> +
> +stop_httpd
> +
> test_done
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 1:22 PM, Jeff King wrote: > On Tue, Jun 26, 2018 at 04:17:08PM -0400, Jeff King wrote: > >> I'm not sure if there's a good solution, though. Even if you retained >> the subshells and instead did a chain-lint inside each subshell, like >> this: > > So obviously that means "I don't think there's a good solution with this > approach". > > That whole final patch simultaneously impresses and nauseates me. Your > commit message says "no attempt is made at properly parsing shell code", > but we come pretty darn close. I almost wonder if we'd be better off > just parsing some heuristic subset and making sure (via review or > linting) that our tests conform. > > Another option is to not enable this slightly-more-dangerous linting by > default. But that would probably rob it of its usefulness, since it > would just fall to some brave soul to later crank up the linting and fix > everybody else's mistakes. This may be a dumb question, but why can't we run under errexit? If we could do that, we wouldn't need the &&-chaining, and bash would parse the shell for us and exit whenever one command failed. (Is the reason for this documented somewhere? I couldn't find it...)
Re: [PATCH] rebase -i: Fix white space in comments
Let's Cc: Wink, who authored the commit mentioned as culprit in the commit
message.
On Tue, 26 Jun 2018, dana wrote:
> Fix a trivial white-space issue introduced by commit d48f97aa8
> ("rebase: reindent function git_rebase__interactive", 2018-03-23). This
> affected the instructional comments displayed in the editor during an
> interactive rebase.
>
> Signed-off-by: dana
> ---
>
> Sorry if i've done any of this wrong; i've never used this work-flow
> before. In any case, if it's not immediately obvious, this is the issue
> i mean to fix:
>
> BEFORE (2.17.1):
>
> # If you remove a line here THAT COMMIT WILL BE LOST.
> #
> # However, if you remove everything, the rebase will be aborted.
> #
> # Note that empty commits are commented out
>
> AFTER (2.18.0):
>
> # If you remove a line here THAT COMMIT WILL BE LOST.
> #
> # However, if you remove everything, the rebase will be aborted.
> #
> #
> # Note that empty commits are commented out
>
> The 2.18.0 version is particularly irritating because many editors
> highlight the trailing tab in the penultimate line as a white-space
> error.
>
> Aside: It's not a new thing, but i've always felt like that last line
> should end in a full stop. Maybe i'll send a patch for that too.
>
> Cheers,
> dana
>
> git-rebase--interactive.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 299ded213..a31af6d4c 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -222,9 +222,9 @@ $comment_char $(eval_ngettext \
> EOF
> append_todo_help
> gettext "
> - However, if you remove everything, the rebase will be aborted.
> +However, if you remove everything, the rebase will be aborted.
>
> - " | git stripspace --comment-lines >>"$todo"
> +" | git stripspace --comment-lines >>"$todo"
>
> if test -z "$keep_empty"
> then
> --
> 2.18.0
>
>
Re: [PATCH v5 2/8] upload-pack: implement ref-in-want
Brandon Williams writes:
> +wanted-refs section
> + * This section is only included if the client has requested a
> + ref using a 'want-ref' line and if a packfile section is also
> + included in the response.
> +
> + * Always begins with the section header "wanted-refs".
> +
> + * The server will send a ref listing (" ") for
> + each reference requested using 'want-ref' lines.
> +
> + * The server SHOULD NOT send any refs which were not requested
> + using 'want-ref' lines and a client MUST ignore refs which
> + weren't requested.
Just being curious, but the above feels the other way around. Why
are we being more lenient to writers of broken server than writers
of broken clients? The number of installations they need to take
back and replace is certainly lower for the former, which means
that, if exchanges of unsoliclited refs are unwanted, clients should
notice and barf (or warn) if the server misbehaves, and the server
should be forbidden from sending unsolicited refs, no?
> diff --git a/t/t5703-upload-pack-ref-in-want.sh
> b/t/t5703-upload-pack-ref-in-want.sh
> new file mode 100755
> index 0..0ef182970
> --- /dev/null
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -0,0 +1,153 @@
> +#!/bin/sh
> +
> +test_description='upload-pack ref-in-want'
> +
> +. ./test-lib.sh
> +
> +get_actual_refs() {
Style. "get_actual_refs () {"
> + sed -n '/wanted-refs/,/0001/p' unpack >actual_refs
Unnecessary piping of sed output into another sed invocation?
sed -n -e '/wanted-refs/,/0001/{
/wanted-refs/d
/0001/d
p
}'
or something like that?
Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells
On Tue, Jun 26, 2018 at 3:38 PM Junio C Hamano wrote:
> I first looked at 29/29 and got heavily inclined to reject that
> step, and then continued reading from 1/29 to around 15/29.
>
> I like these earlier changes that fix existing breakage, of course.
> I also like many of the changes that simplify and/or modernise the
> test scripts very much, but they are unusable as-is as long as their
> justification is "chain-lint will start barfing on these constructs".
Sorry, I'm having difficulty understanding.
Are you saying that you don't want patches which exist merely to
pacify --chain-lint? (For instance, 2/29 "t0001: use "{...}" block
around "||" expression rather than subshell".)
Or are you saying that you don't like how the commit messages are
worded, and that they should instead emphasize that the change is good
for its own sake, without mentioning --chain-lint?
Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect
Hi Chris,
On Tue, 26 Jun 2018, Christian Couder wrote:
> On Tue, Jun 26, 2018 at 4:10 PM, Johannes Schindelin
> wrote:
> >
> > The point, for me, is: if this test fails, at some stage in the
> > future, for any reason, it will be a major pain to even dissect what
> > the test is supposed to do.
>
> I think the test is quite simple and there is an ascii picture, so it is
> not so difficult to understand.
So I tell you that I find it hard to understand and you try to refute this
review by... saying that it is quite simple to understand?
That's not really a basis for discussion there, if you do not even
acknowledge that both Junio and I pointed out a serious flaw in the patch.
I have no idea how you think we can reach consensus if you just deny that
there is a problem *when I just demonstrated that there is a
problem*?!?!?!
> I know that we should try to do better, but here I am mostly
> interested in moving forward a feature that people have requested for
> ages, not cleaning up those tests. If someone else like you or Junio
> thinks that it would be a good time to clean things up a bit, then I
> am ok with it, but that's not my main goal here.
This test is not good. It is not good because if even a long-time
contributor such as myself finds it too hard to understand.
So you cannot just force it forward.
> > - the graph is definitely written in an unnecessarily different format
> > than in the same test script!!!
>
> Just above you complain about things that are similar to the previous
> tests and now you complain about things that are different...
You misunderstood. I complained that things are *not* similar to previous
tests. I said the exact opposite of what you understood.
> > - speaking of the graph: there is a perfectly fine commit graph already.
> > Why on earth is it not reused?
>
> Perhaps because it is more complex than needed to test this feature
> and/or to understand what happens.
What? Have you even *bothered* to look at the graphs?
If at all, the newly introduced is *less* complex (except for the
pointlessly confusing commit names). Both graphs have a branch point, a
tip commit, and a first-parent and a second-parent lineage.
So the new test introduces new commits for no good reason whatsoever, with
a diagram that is unnecessarily different from the one that is already in
the file (horizontal layout instead of vertical, and the horizontal layout
disagrees even with the existing other tests' horizontal graph diagram).
> > In this particular case it even feels as if this test is not even testing
> > what it should test at all:
> >
> > - it should verify that all of the commits in the first parent lineage are
> > part of the list
>
> It does that.
Yes, it does that, and you can verify that it does by, uhm, spending 10
minutes pouring over the diagram.
In my proposed test case, you see the same after reading the simple loop
and from the comment and the line count test, i.e. after spending 10
seconds.
So your "It does that" is a bit inappropriate. Of course your test does
that. In a very convoluted way. And I demonstrated that it does not have
to be convoluted at all.
> > - it should verify that none of the other commits are in the list
>
> It does that too.
Same here. You miss the point. It is obvious in my proposed test case that
it does that. In your proposed test case it is everything but obvious.
> > And that is really all there is to test.
>
> I don't agree. I think that when possible, especially when testing
> plumbing commands like rev-list, it is a good thing to test as many
> things as possible at once.
Then you do not understand the main purpose of regression tests, which is
to not only catch, but also to fix regressions. And your test code seems
to be designed to make the latter very, very hard. Just like your response
to my review almost appears to be designed to be defiant rather than
constructive.
If you want to accept my help in making your code better, we can continue.
Otherwise I am afraid that your patch is stuck in the "not good enough"
pile.
> > You can even write that in a much
> > easier way:
> >
> > -- snip --
> > test_expect_success '--first-parent --bisect-all lists correct revs' '
> > git rev-list --first-parent --bisect-all F..E >revs &&
> > # E and e1..e8 need to be shown, F and f1..f4 not
> > test_line_count = 9 revs &&
> > for rev in E e1 e2 e3 e4 e5 e6 e7 e8
> > do
> > grep "^$(git rev-parse $rev) " revs || {
> > echo "$rev not shown" >&2 &&
> > return 1
> > }
> > done
> > '
> > -- snap --
> >
> > To test more precisely for the order or the distance would be both
> > overkill and likely to be unreadable.
>
> I don't think the previous version was either overkill or unreadable.
Okay, this is too tedious. Seriously, why do you make it so unpleasant to
help you improve the patch.
> Yeah it had other (potenti
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 5:01 PM Jeff King wrote:
> On Tue, Jun 26, 2018 at 04:46:18PM -0400, Eric Sunshine wrote:
> > Some of these dangers can be de-thoothed during the linting phase by
> > defining do-nothing shell functions:
> >
> > cp () { :; }
> > mv () { :; }
> > ln () { :; }
> >
> > That, at least, makes the scariest case ("rm") much less so.
>
> Now that's an interesting idea. We can't catch every dangerous action
> (notably ">" would be hard to override), but it should be pretty cheap
> to cover some obvious ones.
Taking the idea a bit further, the 'sed' script could also throw away
strings of "../" inside subshells, which would help defang the more
difficult cases, like "echo x >../git.c". There are pathological
cases, of course, which it wouldn't catch:
P=../git.c
test_expect_success 'foo' '
(
cd dir &&
echo x >$P
)
'
but it does help mitigate the issue for the most typical cases.
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Jeff King writes: > One way this series might be worse in practice is that we tend not to > change process state too much outside of the subshells. > ... > Whereas once you start collapsing subshells into the main logic chain, > there's a very high chance that the subshell is doing a "cd", since > that's typically the main reason for the subshell in the first place. Exactly. I should have mentioned this when I responded to save a round-trip.
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 04:46:18PM -0400, Eric Sunshine wrote:
> > I'm not sure if there's a good solution, though. Even if you retained
> > the subshells and instead did a chain-lint inside each subshell, like
> > this:
> >
> > (exit 117) &&
> > one &&
> > (
> > (exit 117) &&
> > cd foo
> > two
> > ) &&
> > three
>
> I thought of that too, but the inner (exit 117) doesn't even get
> invoked unless there is &&-chain breakage somewhere above that point
> (for instance, if "one" lacks "&&"), so the inner (exit 117) doesn't
> participate in the linting process at all.
Oh, right. Not only does it not fix the problem, it's totally
unworkable. :)
> Some of these dangers can be de-thoothed during the linting phase by
> defining do-nothing shell functions:
>
> cp () { :; }
> mv () { :; }
> ln () { :; }
>
> That, at least, makes the scariest case ("rm") much less so.
Now that's an interesting idea. We can't catch every dangerous action
(notably ">" would be hard to override), but it should be pretty cheap
to cover some obvious ones.
-Peff
Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually
Am 26.06.2018 um 20:14 schrieb Eric Sunshine: On Tue, Jun 26, 2018 at 2:06 PM Johannes Sixt wrote: Hence, these lines should actually be p4 help client && ! p4 help nosuchcommand Thanks for the comment; you're right, of course. I'll certainly make this change if I have to re-roll for some other reason, but do you feel that this itself is worth a re-roll? Not worth a re-roll IMO. -- Hannes
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 4:22 PM Jeff King wrote: > So obviously that means "I don't think there's a good solution with this > approach". > > That whole final patch simultaneously impresses and nauseates me. Your > commit message says "no attempt is made at properly parsing shell code", > but we come pretty darn close. I almost wonder if we'd be better off > just parsing some heuristic subset and making sure (via review or > linting) that our tests conform. I'm not sure I agree with "come pretty darn close", but your idea is an interesting one. It would sidestep the concern with "rm -fr" and friends (though it will probably still nauseate you). Let me cogitate about it a bit... > Another option is to not enable this slightly-more-dangerous linting by > default. But that would probably rob it of its usefulness, since it > would just fall to some brave soul to later crank up the linting and fix > everybody else's mistakes. I considered that, as well, and came to the same conclusion.
Re: [PATCH v2 5/6] submodule-config: pass repository as argument to config_from_gitmodules
On Tue, 26 Jun 2018 13:15:33 -0700 Junio C Hamano wrote: > Antonio Ospite writes: > > > Generlize config_from_gitmodules to accept a repository as an argument. > > generalize??? > Of course I was going to miss a typo in the first word of the commit message :| If this is the only change, I'd ask you to amend it when applying the patch, if it's not too much trouble. If instead I have to add also the comments about the new public functions in submodule-config.c, as you asked for patch 2/6, I can send a v3 and fix the typo there. Thanks, Antonio -- Antonio Ospite https://ao2.it https://twitter.com/ao2it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing?
[PATCH v5 0/8] ref-in-want
Changes in v5: * Added a comment explaining the one-time-sed.sh script * Added a number of tests per reviewer feedback * Fixed a typo in documentation Brandon Williams (8): test-pkt-line: add unpack-sideband subcommand upload-pack: implement ref-in-want upload-pack: test negotiation with changing repository fetch: refactor the population of peer ref OIDs fetch: refactor fetch_refs into two functions fetch: refactor to make function args narrower fetch-pack: put shallow info in output parameter fetch-pack: implement ref-in-want Documentation/config.txt| 7 + Documentation/technical/protocol-v2.txt | 29 +- builtin/clone.c | 4 +- builtin/fetch.c | 135 + fetch-object.c | 2 +- fetch-pack.c| 50 +++- remote.c| 1 + remote.h| 1 + t/helper/test-pkt-line.c| 33 +++ t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 8 + t/lib-httpd/one-time-sed.sh | 22 ++ t/t5703-upload-pack-ref-in-want.sh | 351 transport-helper.c | 6 +- transport-internal.h| 9 +- transport.c | 34 ++- transport.h | 3 +- upload-pack.c | 66 + 18 files changed, 687 insertions(+), 75 deletions(-) create mode 100644 t/lib-httpd/one-time-sed.sh create mode 100755 t/t5703-upload-pack-ref-in-want.sh -- 2.18.0.rc2.346.g013aa6912e-goog
Re: [PATCH v2 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules
On Tue, 26 Jun 2018 13:11:40 -0700
Junio C Hamano wrote:
> Antonio Ospite writes:
>
> > Add a helper function to make it clearer that retrieving 'fetch'
> > configuration from the .gitmodules file is a special case supported
> > solely for backward compatibility purposes.
> > ...
>
> Then perhaps the new public function deserves a comment stating
> that?
>
Hi Junio,
a comment about that is added to submodule-config.h in patch 4/6 in
place of the comment about config_from_gitmodules.
I can add a note here as well if that one is not enough.
[...]
> > +void fetch_config_from_gitmodules(int *max_children, int
> > *recurse_submodules)
> > +{
> > + struct fetch_config config = {
> > + .max_children = max_children,
> > + .recurse_submodules = recurse_submodules
> > + };
>
> We started using designated initializers some time ago, and use of
> it improves readability of something like this ;-)
>
Ah, TBH I didn't even consider whether it was allowed in git code, I
just used the construct out of habit.
Ciao,
Antonio
--
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
[PATCH v5 7/8] fetch-pack: put shallow info in output parameter
Expand the transport fetch method signature, by adding an output
parameter, to allow transports to return information about the refs they
have fetched. Then communicate shallow status information through this
mechanism instead of by modifying the input list of refs.
This does require clients to sometimes generate the ref map twice: once
from the list of refs provided by the remote (as is currently done) and
potentially once from the new list of refs that the fetch mechanism
provides.
Signed-off-by: Brandon Williams
---
builtin/clone.c | 4 ++--
builtin/fetch.c | 28
fetch-object.c | 2 +-
fetch-pack.c | 15 ---
transport-helper.c | 6 --
transport-internal.h | 9 -
transport.c | 34 --
transport.h | 3 ++-
8 files changed, 77 insertions(+), 24 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae8..8f86d99c5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1155,7 +1155,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
}
if (!is_local && !complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
remote_head = find_ref_by_name(refs, "HEAD");
remote_head_points_at =
@@ -1197,7 +1197,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (is_local)
clone_local(path, git_dir);
else if (refs && complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
update_remote_refs(refs, mapped_refs, remote_head_points_at,
branch_top.buf, reflog_msg.buf, transport,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index bda00e826..0347cf016 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -946,11 +946,13 @@ static int quickfetch(struct ref *ref_map)
return check_connected(iterate_ref_map, &rm, &opt);
}
-static int fetch_refs(struct transport *transport, struct ref *ref_map)
+static int fetch_refs(struct transport *transport, struct ref *ref_map,
+ struct ref **updated_remote_refs)
{
int ret = quickfetch(ref_map);
if (ret)
- ret = transport_fetch_refs(transport, ref_map);
+ ret = transport_fetch_refs(transport, ref_map,
+ updated_remote_refs);
if (!ret)
/*
* Keep the new pack's ".keep" file around to allow the caller
@@ -1112,7 +1114,7 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- if (!fetch_refs(transport, ref_map))
+ if (!fetch_refs(transport, ref_map, NULL))
consume_refs(transport, ref_map);
if (gsecondary) {
@@ -1128,6 +1130,7 @@ static int do_fetch(struct transport *transport,
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
const struct ref *remote_refs;
+ struct ref *updated_remote_refs = NULL;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
@@ -1178,7 +1181,24 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
+
+ if (fetch_refs(transport, ref_map, &updated_remote_refs)) {
+ free_refs(ref_map);
+ retcode = 1;
+ goto cleanup;
+ }
+ if (updated_remote_refs) {
+ /*
+* Regenerate ref_map using the updated remote refs. This is
+* to account for additional information which may be provided
+* by the transport (e.g. shallow info).
+*/
+ free_refs(ref_map);
+ ref_map = get_ref_map(transport->remote, updated_remote_refs,
rs,
+ tags, &autotags);
+ free_refs(updated_remote_refs);
+ }
+ if (consume_refs(transport, ref_map)) {
free_refs(ref_map);
retcode = 1;
goto cleanup;
diff --git a/fetch-object.c b/fetch-object.c
index 853624f81..48fe63dd6 100644
--- a/fetch-object.c
+++ b/fetch-object.c
@@ -19,7 +19,7 @@ static void fetch_refs(const char *remote_name, struct ref
*ref)
transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
transport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, "1");
[PATCH v5 8/8] fetch-pack: implement ref-in-want
Implement ref-in-want on the client side so that when a server supports
the "ref-in-want" feature, a client will send "want-ref" lines for each
reference the client wants to fetch. This feature allows clients to
tolerate inconsistencies that exist when a remote repository's refs
change during the course of negotiation.
This allows a client to request to request a particular ref without
specifying the OID of the ref. This means that instead of hitting an
error when a ref no longer points at the OID it did at the beginning of
negotiation, negotiation can continue and the value of that ref will be
sent at the termination of negotiation, just before a packfile is sent.
More information on the ref-in-want feature can be found in
Documentation/technical/protocol-v2.txt.
Signed-off-by: Brandon Williams
---
fetch-pack.c | 35 +++-
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 130 +
4 files changed, 164 insertions(+), 3 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 73890b894..3a18f5bcd 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1102,9 +1102,10 @@ static void add_shallow_requests(struct strbuf *req_buf,
static void add_wants(const struct ref *wants, struct strbuf *req_buf)
{
+ int use_ref_in_want = server_supports_feature("fetch", "ref-in-want",
0);
+
for ( ; wants ; wants = wants->next) {
const struct object_id *remote = &wants->old_oid;
- const char *remote_hex;
struct object *o;
/*
@@ -1122,8 +1123,10 @@ static void add_wants(const struct ref *wants, struct
strbuf *req_buf)
continue;
}
- remote_hex = oid_to_hex(remote);
- packet_buf_write(req_buf, "want %s\n", remote_hex);
+ if (!use_ref_in_want || wants->exact_oid)
+ packet_buf_write(req_buf, "want %s\n",
oid_to_hex(remote));
+ else
+ packet_buf_write(req_buf, "want-ref %s\n", wants->name);
}
}
@@ -1334,6 +1337,29 @@ static void receive_shallow_info(struct fetch_pack_args
*args,
args->deepen = 1;
}
+static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
+{
+ process_section_header(reader, "wanted-refs", 0);
+ while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+ struct object_id oid;
+ const char *end;
+ struct ref *r = NULL;
+
+ if (parse_oid_hex(reader->line, &oid, &end) || *end++ != ' ')
+ die("expected wanted-ref, got '%s'", reader->line);
+
+ for (r = refs; r; r = r->next) {
+ if (!strcmp(end, r->name)) {
+ oidcpy(&r->old_oid, &oid);
+ break;
+ }
+ }
+ }
+
+ if (reader->status != PACKET_READ_DELIM)
+ die("error processing wanted refs: %d", reader->status);
+}
+
enum fetch_state {
FETCH_CHECK_LOCAL = 0,
FETCH_SEND_REQUEST,
@@ -1408,6 +1434,9 @@ static struct ref *do_fetch_pack_v2(struct
fetch_pack_args *args,
if (process_section_header(&reader, "shallow-info", 1))
receive_shallow_info(args, &reader);
+ if (process_section_header(&reader, "wanted-refs", 1))
+ receive_wanted_refs(&reader, ref);
+
/* get the pack */
process_section_header(&reader, "packfile", 0);
if (get_pack(args, fd, pack_lockfile))
diff --git a/remote.c b/remote.c
index abe80c139..2c2376fff 100644
--- a/remote.c
+++ b/remote.c
@@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
if (refspec->exact_sha1) {
ref_map = alloc_ref(name);
get_oid_hex(name, &ref_map->old_oid);
+ ref_map->exact_oid = 1;
} else {
ref_map = get_remote_ref(remote_refs, name);
}
diff --git a/remote.h b/remote.h
index 45ecc6cef..976292152 100644
--- a/remote.h
+++ b/remote.h
@@ -73,6 +73,7 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
+ exact_oid:1,
deletion:1;
enum {
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index a4fe0e7e4..392be4959 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -204,6 +204,18 @@ test_expect_success 'server is initially ahead - no ref in
want' '
grep "ERR upload-pack: not our ref" err
'
+test_expect_success 'server is initially ahead - ref in want' '
+ git -C "$RE
[PATCH v5 6/8] fetch: refactor to make function args narrower
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 52 -
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 2fabfed0e..bda00e826 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -254,9 +254,9 @@ static int will_fetch(struct ref **head, const unsigned
char *sha1)
return 0;
}
-static void find_non_local_tags(struct transport *transport,
- struct ref **head,
- struct ref ***tail)
+static void find_non_local_tags(const struct ref *refs,
+ struct ref **head,
+ struct ref ***tail)
{
struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct string_list remote_refs = STRING_LIST_INIT_NODUP;
@@ -264,7 +264,7 @@ static void find_non_local_tags(struct transport *transport,
struct string_list_item *item = NULL;
for_each_ref(add_existing, &existing_refs);
- for (ref = transport_get_remote_refs(transport, NULL); ref; ref =
ref->next) {
+ for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
@@ -338,7 +338,8 @@ static void find_non_local_tags(struct transport *transport,
string_list_clear(&remote_refs, 0);
}
-static struct ref *get_ref_map(struct transport *transport,
+static struct ref *get_ref_map(struct remote *remote,
+ const struct ref *remote_refs,
struct refspec *rs,
int tags, int *autotags)
{
@@ -346,27 +347,11 @@ static struct ref *get_ref_map(struct transport
*transport,
struct ref *rm;
struct ref *ref_map = NULL;
struct ref **tail = &ref_map;
- struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail = &orefs;
struct string_list existing_refs = STRING_LIST_INIT_DUP;
- const struct ref *remote_refs;
-
- if (rs->nr)
- refspec_ref_prefixes(rs, &ref_prefixes);
- else if (transport->remote && transport->remote->fetch.nr)
- refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);
-
- if (ref_prefixes.argc &&
- (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
- argv_array_push(&ref_prefixes, "refs/tags/");
- }
-
- remote_refs = transport_get_remote_refs(transport, &ref_prefixes);
-
- argv_array_clear(&ref_prefixes);
if (rs->nr) {
struct refspec *fetch_refspec;
@@ -403,7 +388,7 @@ static struct ref *get_ref_map(struct transport *transport,
if (refmap.nr)
fetch_refspec = &refmap;
else
- fetch_refspec = &transport->remote->fetch;
+ fetch_refspec = &remote->fetch;
for (i = 0; i < fetch_refspec->nr; i++)
get_fetch_map(ref_map, &fetch_refspec->items[i],
&oref_tail, 1);
@@ -411,7 +396,6 @@ static struct ref *get_ref_map(struct transport *transport,
die("--refmap option is only meaningful with command-line
refspec(s).");
} else {
/* Use the defaults */
- struct remote *remote = transport->remote;
struct branch *branch = branch_get(NULL);
int has_merge = branch_has_merge_config(branch);
if (remote &&
@@ -450,7 +434,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* also fetch all tags */
get_fetch_map(remote_refs, tag_refspec, &tail, 0);
else if (tags == TAGS_DEFAULT && *autotags)
- find_non_local_tags(transport, &ref_map, &tail);
+ find_non_local_tags(remote_refs, &ref_map, &tail);
/* Now append any refs to be updated opportunistically: */
*tail = orefs;
@@ -1143,6 +1127,8 @@ static int do_fetch(struct transport *transport,
struct ref *ref_map;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
+ const struct ref *remote_refs;
+ struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1158,7 +1144,21 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
- ref_map = get_ref_map(transport, rs, tags, &autotags);
+ if (rs->nr)
+ refspec_ref_prefixe
[PATCH v5 3/8] upload-pack: test negotiation with changing repository
Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.
As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.
Signed-off-by: Brandon Williams
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf| 8
t/lib-httpd/one-time-sed.sh| 22 ++
t/t5703-upload-pack-ref-in-want.sh | 68 ++
4 files changed, 99 insertions(+)
create mode 100644 t/lib-httpd/one-time-sed.sh
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465..84f8efdd4 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -132,6 +132,7 @@ prepare_httpd() {
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
install_script error.sh
+ install_script one-time-sed.sh
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 724d9ae46..fe68d37bb 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -111,9 +111,14 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
+
+ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+ SetEnv GIT_HTTP_EXPORT_ALL
+
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error/ error.sh/
+ScriptAliasMatch /one_time_sed/(.*) one-time-sed.sh/$1
Options FollowSymlinks
@@ -123,6 +128,9 @@ ScriptAlias /error/ error.sh/
Options ExecCGI
+
+ Options ExecCGI
+
Options ExecCGI
diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
new file mode 100644
index 0..8a9a5aca0
--- /dev/null
+++ b/t/lib-httpd/one-time-sed.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+# If "one-time-sed" exists in $HTTPD_ROOT_PATH, run sed on the HTTP response,
+# using the contents of "one-time-sed" as the sed command to be run. If the
+# response was modified as a result, delete "one-time-sed" so that subsequent
+# HTTP responses are no longer modified.
+#
+# This can be used to simulate the effects of the repository changing in
+# between HTTP request-response pairs.
+if [ -e one-time-sed ]; then "$GIT_EXEC_PATH/git-http-backend" >out
+
+ sed "$(cat one-time-sed)" out_modified
+
+ if diff out out_modified >/dev/null; then
+ cat out
+ else
+ cat out_modified
+ rm one-time-sed
+ fi
+else
+ "$GIT_EXEC_PATH/git-http-backend"
+fi
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index 0ef182970..a4fe0e7e4 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -150,4 +150,72 @@ test_expect_success 'want-ref with ref we already have
commit for' '
check_output
'
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for change-while-negotiating test' '
+ (
+ git init "$REPO" &&
+ cd "$REPO" &&
+ >.git/git-daemon-export-ok &&
+ test_commit m1 &&
+ git tag -d m1 &&
+
+ # Local repo with many commits (so that negotiation will take
+ # more than 1 request/response pair)
+ git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo";
"$LOCAL_PRISTINE" &&
+ cd "$LOCAL_PRISTINE" &&
+ git checkout -b side &&
+ for i in $(seq 1 33); do test_commit s$i; done &&
+
+ # Add novel commits to upstream
+ git checkout master &&
+ cd "$REPO" &&
+ test_commit m2 &&
+ test_commit m3 &&
+ git tag -d m2 m3
+ ) &&
+ git -C "$LOCAL_PRISTINE" remote set-url origin
"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo"; &&
+ git -C "$LOCAL_PRISTINE" config protocol.version 2
+'
+
+inconsistency() {
+ # Simulate that the server initially reports $2 as the ref
+ # corresponding to $1, and after that, $1 as the ref corresponding to
+ # $1. This corresponds to the real-life situation where the server's
+ # repository appears to change during negotiation, for example, when
+ # different servers in a load-balancing arrangement serve (stateless)
+ # RPCs during a single negotiation.
+ printf "s/%s/%s/" \
+ $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
+ $(git -C "$REPO" rev-parse $2
[PATCH v5 2/8] upload-pack: implement ref-in-want
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement and
there exists replication delay.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref " parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams
---
Documentation/config.txt| 7 ++
Documentation/technical/protocol-v2.txt | 29 -
t/t5703-upload-pack-ref-in-want.sh | 153
upload-pack.c | 66 ++
4 files changed, 254 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a..fb1dd7428 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it
is seen in the
repository-level config (this is a safety measure against fetching from
untrusted repositories).
+uploadpack.allowRefInWant::
+ If this option is set, `upload-pack` will support the `ref-in-want`
+ feature of the protocol version 2 `fetch` command. This feature
+ is intended for the benefit of load-balanced servers which may
+ not have the same view of what OIDs their refs point to due to
+ replication delay.
+
url..insteadOf::
Any URL that starts with this value will be rewritten to
start, instead, with . In cases where some site serves a
diff --git a/Documentation/technical/protocol-v2.txt
b/Documentation/technical/protocol-v2.txt
index 49bda76d2..b53cfc6ce 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -299,12 +299,21 @@ included in the client's request:
for use with partial clone and partial fetch operations. See
`rev-list` for possible "filter-spec" values.
+If the 'ref-in-want' feature is advertised, the following argument can
+be included in the client's request as well as the potential addition of
+the 'wanted-refs' section in the server's response as explained below.
+
+want-ref
+ Indicates to the server that the client wants to retrieve a
+ particular ref, where is the full name of a ref on the
+ server.
+
The response of `fetch` is broken into a number of sections separated by
delimiter packets (0001), with each section beginning with its section
header.
output = *section
-section = (acknowledgments | shallow-info | packfile)
+section = (acknowledgments | shallow-info | wanted-refs | packfile)
(flush-pkt | delim-pkt)
acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -319,6 +328,10 @@ header.
shallow = "shallow" SP obj-id
unshallow = "unshallow" SP obj-id
+wanted-refs = PKT-LINE("wanted-refs" LF)
+ *PKT-LINE(wanted-ref LF)
+wanted-ref = obj-id SP refname
+
packfile = PKT-LINE("packfile" LF)
*PKT-LINE(%x01-03 *%x00-ff)
@@ -379,6 +392,20 @@ header.
* This section is only included if a packfile section is also
included in the response.
+wanted-refs section
+ * This section is only included if the client has requested a
+ ref using a 'want-ref' line and if a packfile section is also
+ included in the response.
+
+ * Always begins with the section header "wanted-refs".
+
+ * The server will send a ref listing (" ") for
+ each reference requested using 'want-ref' lines.
+
+ * The server SHOULD NOT send any refs which were not requested
+ using 'want-ref' lines and a client MUST ignore refs which
+ weren't requested.
+
packfile section
* This section is only included if the client has sent 'want'
lines in its request and either requested that no more
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
new file mode 100755
index 0..0ef182970
--- /dev/null
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -0,0 +1,153 @@
+#!/bin/sh
+
+test_description='upload-pack ref-in-want'
+
+. ./test-lib.sh
+
+get_actual_refs() {
+ sed -n '/wanted-refs/,/0001/p' actual_refs
+}
+
+get_actual_commits() {
+ sed -n '/packfile/,//p' o.pack &&
+ git index-pack o.pack &&
+ git verify-pack -v o.idx | grep commit | cut -c-40 | sort
>actual_commits
+}
+
+check_output() {
+ get_actual_refs &&
+ tes
[PATCH v5 4/8] fetch: refactor the population of peer ref OIDs
Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..545635448 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -351,6 +351,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail = &orefs;
+ struct string_list existing_refs = STRING_LIST_INIT_DUP;
const struct ref *remote_refs;
if (rs->nr)
@@ -458,7 +459,23 @@ static struct ref *get_ref_map(struct transport *transport,
tail = &rm->next;
}
- return ref_remove_duplicates(ref_map);
+ ref_map = ref_remove_duplicates(ref_map);
+
+ for_each_ref(add_existing, &existing_refs);
+ for (rm = ref_map; rm; rm = rm->next) {
+ if (rm->peer_ref) {
+ struct string_list_item *peer_item =
+ string_list_lookup(&existing_refs,
+ rm->peer_ref->name);
+ if (peer_item) {
+ struct object_id *old_oid = peer_item->util;
+ oidcpy(&rm->peer_ref->old_oid, old_oid);
+ }
+ }
+ }
+ string_list_clear(&existing_refs, 1);
+
+ return ref_map;
}
#define STORE_REF_ERROR_OTHER 1
@@ -1110,14 +1127,10 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
static int do_fetch(struct transport *transport,
struct refspec *rs)
{
- struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct ref *ref_map;
- struct ref *rm;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
- for_each_ref(add_existing, &existing_refs);
-
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
tags = TAGS_SET;
@@ -1136,18 +1149,6 @@ static int do_fetch(struct transport *transport,
if (!update_head_ok)
check_not_current_branch(ref_map);
- for (rm = ref_map; rm; rm = rm->next) {
- if (rm->peer_ref) {
- struct string_list_item *peer_item =
- string_list_lookup(&existing_refs,
- rm->peer_ref->name);
- if (peer_item) {
- struct object_id *old_oid = peer_item->util;
- oidcpy(&rm->peer_ref->old_oid, old_oid);
- }
- }
- }
-
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1183,7 +1184,6 @@ static int do_fetch(struct transport *transport,
}
cleanup:
- string_list_clear(&existing_refs, 1);
return retcode;
}
--
2.18.0.rc2.346.g013aa6912e-goog
[PATCH v5 5/8] fetch: refactor fetch_refs into two functions
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them. This is in preparation
for allowing additional processing of the fetched refs before updating
the local ref store.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 545635448..2fabfed0e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -968,9 +968,21 @@ static int fetch_refs(struct transport *transport, struct
ref *ref_map)
if (ret)
ret = transport_fetch_refs(transport, ref_map);
if (!ret)
- ret |= store_updated_refs(transport->url,
- transport->remote->name,
- ref_map);
+ /*
+* Keep the new pack's ".keep" file around to allow the caller
+* time to update refs to reference the new objects.
+*/
+ return 0;
+ transport_unlock_pack(transport);
+ return ret;
+}
+
+/* Update local refs based on the ref values fetched from a remote */
+static int consume_refs(struct transport *transport, struct ref *ref_map)
+{
+ int ret = store_updated_refs(transport->url,
+transport->remote->name,
+ref_map);
transport_unlock_pack(transport);
return ret;
}
@@ -1116,7 +1128,8 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- fetch_refs(transport, ref_map);
+ if (!fetch_refs(transport, ref_map))
+ consume_refs(transport, ref_map);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1165,7 +1178,7 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map)) {
+ if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
free_refs(ref_map);
retcode = 1;
goto cleanup;
--
2.18.0.rc2.346.g013aa6912e-goog
[PATCH v5 1/8] test-pkt-line: add unpack-sideband subcommand
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams
---
t/helper/test-pkt-line.c | 33 +
1 file changed, 33 insertions(+)
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 0f19e53c7..30775f986 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -1,3 +1,4 @@
+#include "cache.h"
#include "pkt-line.h"
static void pack_line(const char *line)
@@ -48,6 +49,36 @@ static void unpack(void)
}
}
+static void unpack_sideband(void)
+{
+ struct packet_reader reader;
+ packet_reader_init(&reader, 0, NULL, 0,
+ PACKET_READ_GENTLE_ON_EOF |
+ PACKET_READ_CHOMP_NEWLINE);
+
+ while (packet_reader_read(&reader) != PACKET_READ_EOF) {
+ int band;
+ int fd;
+
+ switch (reader.status) {
+ case PACKET_READ_EOF:
+ break;
+ case PACKET_READ_NORMAL:
+ band = reader.line[0] & 0xff;
+ if (band < 1 || band > 2)
+ die("unexpected side band %d", band);
+ fd = band;
+
+ write_or_die(fd, reader.line + 1, reader.pktlen - 1);
+ break;
+ case PACKET_READ_FLUSH:
+ return;
+ case PACKET_READ_DELIM:
+ break;
+ }
+ }
+}
+
int cmd_main(int argc, const char **argv)
{
if (argc < 2)
@@ -57,6 +88,8 @@ int cmd_main(int argc, const char **argv)
pack(argc - 2, argv + 2);
else if (!strcmp(argv[1], "unpack"))
unpack();
+ else if (!strcmp(argv[1], "unpack-sideband"))
+ unpack_sideband();
else
die("invalid argument '%s'", argv[1]);
--
2.18.0.rc2.346.g013aa6912e-goog
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 4:17 PM Jeff King wrote:
> On Tue, Jun 26, 2018 at 03:52:54PM -0400, Eric Sunshine wrote:
> > So, this isn't a new problem introduced by this series, though this
> > series may exacerbate it.
>
> Whereas once you start collapsing subshells into the main logic chain,
> there's a very high chance that the subshell is doing a "cd", since
> that's typically the main reason for the subshell in the first place.
> And with the current --chain-lint logic, that subshell is either
> executed or not executed as a unit.
>
> Obviously that's a bit of a hand-waving argument. If you've fixed all of
> the existing cases without accidentally deleting your home directory,
> then maybe it's not so likely to be a problem after all.
Indeed, it could be that the "rm -fr" worry is tending toward the
hypothetical. Seasoned developers tend to be pretty careful and
usually avoid indiscriminately loose "rm -fr" invocations, so I'm
somewhat less worried about them. I do share the concern, though, that
newcomers crafting or extending tests could shoot themselves in the
foot with this. However, newcomers are also the ones most likely to
use the "cd foo && bar && cd .." idiom, so they are already at risk.
(As for not blasting my home directory when fixing all the existing
tests, I did run into a few cases where one or two "foreign" files
were deposited into the "t/" directory, but nothing was deleted or
overwritten.)
> I'm not sure if there's a good solution, though. Even if you retained
> the subshells and instead did a chain-lint inside each subshell, like
> this:
>
> (exit 117) &&
> one &&
> (
> (exit 117) &&
> cd foo
> two
> ) &&
> three
I thought of that too, but the inner (exit 117) doesn't even get
invoked unless there is &&-chain breakage somewhere above that point
(for instance, if "one" lacks "&&"), so the inner (exit 117) doesn't
participate in the linting process at all.
> that doesn't really help. The fundamental issue is that we may skip the
> "cd" inside the subshell. Whether it's in a subshell or not, that's
> dangerous. True, we don't run "three" in this case, which is slightly
> better. But it didn't expect to be in a different directory anyway. It's
> running "two" that is dangerous.
Just thinking aloud...
Aside from "rm -fr", there are numerous ways to clobber files
unexpectedly when the "cd" is skipped:
echo x >../git.c
cp x ../git.c
mv x ../git.c
ln [-s] x ../git.c
/bin/rm ../git.c
some-cmd -o ../git.c
Some of these dangers can be de-thoothed during the linting phase by
defining do-nothing shell functions:
cp () { :; }
mv () { :; }
ln () { :; }
That, at least, makes the scariest case ("rm") much less so.
Re: [PATCH v2 0/6] Restrict the usage of config_from_gitmodules to submodule-config
Brandon Williams writes: >> Changes since v1: >> * Remove an extra space before an arrow operator in patch 2 >> * Fix a typo in the commit message of patch 3: s/fetchobjs/fetchjobs >> * Add a note in the commit message of patch 6 about checking the >> worktree before loading .gitmodules >> * Drop patch 7, it was meant as a cleanup but resulted in parsing the >> .gitmodules file twice > > Thanks for making these changes, this version looks good to me! Yup, thanks, both.
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 04:17:08PM -0400, Jeff King wrote: > I'm not sure if there's a good solution, though. Even if you retained > the subshells and instead did a chain-lint inside each subshell, like > this: So obviously that means "I don't think there's a good solution with this approach". That whole final patch simultaneously impresses and nauseates me. Your commit message says "no attempt is made at properly parsing shell code", but we come pretty darn close. I almost wonder if we'd be better off just parsing some heuristic subset and making sure (via review or linting) that our tests conform. Another option is to not enable this slightly-more-dangerous linting by default. But that would probably rob it of its usefulness, since it would just fall to some brave soul to later crank up the linting and fix everybody else's mistakes. -Peff
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 03:52:54PM -0400, Eric Sunshine wrote: > The existing --chain-lint already suffers the same shortcoming. Older > (or even new poorly-written) tests, even without subshells, can fall > victim already: > > (exit $sentinel) && > mkdir -p a/b/c && > cd a/b/c > rm -fr ../../* && > cd ../../.. && > statement4 > > As in your example, 'mkdir' and 'cd' are skipped, but 'rm -fr ../../*' is not. > > This snippet from the commit message of bb79af9d09 (t/test-lib: > introduce --chain-lint option, 2015-03-20): > > When we encounter a failure of this check, we abort the test > script entirely. For one thing, we have no clue which subset > of the commands in the test snippet were actually run. > > suggests that the issue was considered, in some form, even then > (though, it doesn't say explicitly that Peff had the 'rm -fr' case in > mind). > > So, this isn't a new problem introduced by this series, though this > series may exacerbate it. One way this series might be worse in practice is that we tend not to change process state too much outside of the subshells. So if we skip some early commands and execute a later "rm", for example, it tends to be in the same directory (and I think as time goes on we have been cleaning up old tests which did a "cd foo && bar && cd .." into using a subshell). Whereas once you start collapsing subshells into the main logic chain, there's a very high chance that the subshell is doing a "cd", since that's typically the main reason for the subshell in the first place. And with the current --chain-lint logic, that subshell is either executed or not executed as a unit. Obviously that's a bit of a hand-waving argument. If you've fixed all of the existing cases without accidentally deleting your home directory, then maybe it's not so likely to be a problem after all. I'm not sure if there's a good solution, though. Even if you retained the subshells and instead did a chain-lint inside each subshell, like this: (exit 117) && one && ( (exit 117) && cd foo two ) && three that doesn't really help. The fundamental issue is that we may skip the "cd" inside the subshell. Whether it's in a subshell or not, that's dangerous. True, we don't run "three" in this case, which is slightly better. But it didn't expect to be in a different directory anyway. It's running "two" that is dangerous. -Peff
Re: [PATCH v2 5/6] submodule-config: pass repository as argument to config_from_gitmodules
Antonio Ospite writes:
> Generlize config_from_gitmodules to accept a repository as an argument.
generalize???
>
> This is in preparation to reuse the function in repo_read_gitmodules in
> order to have a single point where the '.gitmodules' file is accessed.
>
> Signed-off-by: Antonio Ospite
> ---
> submodule-config.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index cd1f1e06a..602c46af2 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -680,10 +680,10 @@ void submodule_free(struct repository *r)
> * Runs the provided config function on the '.gitmodules' file found in the
> * working directory.
> */
> -static void config_from_gitmodules(config_fn_t fn, void *data)
> +static void config_from_gitmodules(config_fn_t fn, struct repository *repo,
> void *data)
> {
> - if (the_repository->worktree) {
> - char *file = repo_worktree_path(the_repository,
> GITMODULES_FILE);
> + if (repo->worktree) {
> + char *file = repo_worktree_path(repo, GITMODULES_FILE);
> git_config_from_file(fn, file, data);
> free(file);
> }
> @@ -714,7 +714,7 @@ void fetch_config_from_gitmodules(int *max_children, int
> *recurse_submodules)
> .max_children = max_children,
> .recurse_submodules = recurse_submodules
> };
> - config_from_gitmodules(gitmodules_fetch_config, &config);
> + config_from_gitmodules(gitmodules_fetch_config, the_repository,
> &config);
> }
>
> static int gitmodules_update_clone_config(const char *var, const char *value,
> @@ -728,5 +728,5 @@ static int gitmodules_update_clone_config(const char
> *var, const char *value,
>
> void update_clone_config_from_gitmodules(int *max_jobs)
> {
> - config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
> + config_from_gitmodules(gitmodules_update_clone_config, the_repository,
> &max_jobs);
> }
Re: [PATCH v2 4/6] submodule-config: make 'config_from_gitmodules' private
Antonio Ospite writes: > Now that 'config_from_gitmodules' is not used in the open, it can be > marked as private. Nice ;-)
Re: [PATCH v2 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules
Antonio Ospite writes:
> Add a helper function to make it clearer that retrieving 'fetch'
> configuration from the .gitmodules file is a special case supported
> solely for backward compatibility purposes.
> ...
Then perhaps the new public function deserves a comment stating
that?
> +struct fetch_config {
> + int *max_children;
> + int *recurse_submodules;
> +};
> +
> +static int gitmodules_fetch_config(const char *var, const char *value, void
> *cb)
> +{
> + struct fetch_config *config = cb;
> + if (!strcmp(var, "submodule.fetchjobs")) {
> + *(config->max_children) = parse_submodule_fetchjobs(var, value);
> + return 0;
> + } else if (!strcmp(var, "fetch.recursesubmodules")) {
> + *(config->recurse_submodules) =
> parse_fetch_recurse_submodules_arg(var, value);
> + return 0;
> + }
> +
> + return 0;
> +}
> +
> +void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)
> +{
> + struct fetch_config config = {
> + .max_children = max_children,
> + .recurse_submodules = recurse_submodules
> + };
We started using designated initializers some time ago, and use of
it improves readability of something like this ;-)
> + config_from_gitmodules(gitmodules_fetch_config, &config);
> +}
> diff --git a/submodule-config.h b/submodule-config.h
> index 5148801f4..cff297a75 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -66,4 +66,6 @@ int check_submodule_name(const char *name);
> */
> extern void config_from_gitmodules(config_fn_t fn, void *data);
>
> +extern void fetch_config_from_gitmodules(int *max_children, int
> *recurse_submodules);
> +
> #endif /* SUBMODULE_CONFIG_H */
Re: [PATCH 14/29] t: drop subshell with missing &&-chain in favor of simpler construct
On Tue, Jun 26, 2018 at 3:31 PM Junio C Hamano wrote: > Eric Sunshine writes: > > These tests employ a noisy subshell (with missing &&-chain) to feed > > input into Git commands: > > > > (echo a; echo b; echo c) | git some-command ... > > > > Drop the subshell in favor of a simple 'printf': > > > > printf "%s\n" a b c | git some-command ... > > That's called test_write_lines, I think. Yep, that's better. Thanks.
Re: [BUG] url schemes should be case-insensitive
On Tue, Jun 26, 2018 at 02:27:39PM -0400, Jeff King wrote:
> So yeah, we would not want to allow EXT::"rm -rf /" to slip past the
> known-unsafe match. Any normalization should happen before then
> (probably right in transport_helper_init).
>
> Come to think of it, that's already sort-of an issue now. If you have a
> case-insensitive filesystem, then EXT:: is going to pass this check, but
> still run git-remote-ext. We're saved there somewhat by the fact that
> the default is to reject unknown helpers in submodules (otherwise, we'd
> have that horrible submodule bug all over again).
>
> That goes beyond just cases, too. On HFS+ I wonder if I could ask for
> "\u{0200}ext::" and run git-remote-ext.
That should be \u{200c}, of course, to get the correct sneaky character.
And the good news is that no, it doesn't work. We are protected by this
code in transport_get():
/* maybe it is a foreign URL? */
if (url) {
const char *p = url;
while (is_urlschemechar(p == url, *p))
p++;
if (starts_with(p, "::"))
helper = xstrndup(url, p - url);
}
So we'll only allow remote-helper names with valid url characters, which
are basically [A-Za-z0-9+.-]. So I think we probably only have to worry
about true case issues, and not any kind of weird filesystem-specific
behaviors.
-Peff
Re: [GSoC][PATCH 1/1] sequencer: print an error message if append_todo_help() fails
Johannes Schindelin writes: > Hi Alban, > > On Tue, 26 Jun 2018, Alban Gruin wrote: > >> This adds an error when append_todo_help() fails to write its message to >> the todo file. >> >> Signed-off-by: Alban Gruin > > ACK. > > We *may* want to fold that into the commit that adds `append_todo_help()`. Absolutely. This looks more like an "oops, I made a mess and here is a fix on top", and even worse, it does not make an effort to help readers where the mess was made (iow, which commit it goes on to of); it is better to be squashed in. I do not know offhand who Alban's mentors are, but one thing I think is a good thing for them to teach is how to better organize the changes with readers in mind. The author of a patch series knows his or her patches and how they relate to each other a lot better than the readers of patches, who are reading not just his or her patches but the ones from a lot wider set of contributors. Even though append-todo-help and edit-todo may have been developed as separate steps in author's mind, it is criminal to send them as if they are completely separate topics that can independently applied, especially when one depends on the other. It is a lot more helpful to the readers if they were sent as a larger single series, because doing so _will_ tell the readers which order the dependency goes. > And, as I mentioned previously, I would love for that function to be used > as an excuse to introduce the long-overdue `interactive-rebase.c` I am not sure if I like this direction. As newbies are often very bad at coming up with APIs and naming global functions, keeping everything as "static" inside a single sequencer.c tends to avoid contaminating the global namespace.
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jun 26, 2018 at 3:15 PM Junio C Hamano wrote: > so, with --chain-lint, we would transform this > > mkdir -p a/b/c && > ( > cd a/b/c > rm -fr ../../* > ) && > statement 4 > > into this sequence > > (exit $sentinel) && > mkdir -p a/b/c && > cd a/b/c > rm -fr ../../* && > statement 4 > > and then rely on the non-zero exit to cancel all the remainder? > > We didn't create nor cd to the t/trash$num/a/b/c thanks to the && > chain, and end up running rm -fr ../../* from inside t/trash$num? Yes, I did take that into account and, no, I don't have a good answer to the issue. The existing --chain-lint already suffers the same shortcoming. Older (or even new poorly-written) tests, even without subshells, can fall victim already: (exit $sentinel) && mkdir -p a/b/c && cd a/b/c rm -fr ../../* && cd ../../.. && statement4 As in your example, 'mkdir' and 'cd' are skipped, but 'rm -fr ../../*' is not. This snippet from the commit message of bb79af9d09 (t/test-lib: introduce --chain-lint option, 2015-03-20): When we encounter a failure of this check, we abort the test script entirely. For one thing, we have no clue which subset of the commands in the test snippet were actually run. suggests that the issue was considered, in some form, even then (though, it doesn't say explicitly that Peff had the 'rm -fr' case in mind). So, this isn't a new problem introduced by this series, though this series may exacerbate it. Thanks for thinking critically about it.
Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells
Eric Sunshine writes: > The --chain-lint[1] option detects breakage in the top-level &&-chain of > tests. This series undertakes the more complex task of teaching it to > also detect &&-chain breakage within subshells. See patch 29/29 for the > gory details of how that's done. I first looked at 29/29 and got heavily inclined to reject that step, and then continued reading from 1/29 to around 15/29. I like these earlier changes that fix existing breakage, of course. I also like many of the changes that simplify and/or modernise the test scripts very much, but they are unusable as-is as long as their justification is "chain-lint will start barfing on these constructs".
Re: git rerere and diff3
On Tue, Jun 26, 2018 at 9:05 PM Junio C Hamano wrote: > > Nicolas Dechesne writes: > > > i have noticed that merge.conflictstyle has an impact on the rerere > > resolution. looking briefly at the source code, it seems that git > > tries to discard the common ancestor diff3 bits, but what I am seeing > > is that if i do the following then it fails: > > > > 1. from a clean rr-cache state, with merge.conflictsytle=diff3, git > > merge , resolve the conflicts, then commit > > 2. undo the previous merge, remove merge.conflictstyle=diff3 (disable > > diff3) and merge the *same* branch, then rerere won't fix the > > conflicts > > It is possible that the conflict left when making the same merge are > actually different when using these two conflict styles. IOW, if > the merge produces > > <<< > side A > ||| > common > === > side B > >>> > > when diff3 style is chosen, but if the same merge results in > > <<< > side A' > === > side B' > >>> > > where side A' is not identical to side A (or B' and B are not > identical), then we will fail to find the previously recorded > resolution. well, you're rigth.. that's what's happening... === with conflictstyle=merge diff --cc arch/arm64/configs/defconfig index 3cfa8ca26738,d53a1b00ad82.. --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@@ -356,7 -334,7 +357,11 @@@ CONFIG_GPIO_PCA953X= CONFIG_GPIO_PCA953X_IRQ=y CONFIG_GPIO_MAX77620=y CONFIG_POWER_AVS=y ++<<< HEAD +CONFIG_ROCKCHIP_IODOMAIN=y ++=== + CONFIG_QCOM_CPR=y ++>>> tracking-qcomlt-8016-dvfs CONFIG_POWER_RESET_MSM=y CONFIG_POWER_RESET_XGENE=y CONFIG_POWER_RESET_SYSCON=y === with conflictstyle=diff3 diff --cc arch/arm64/configs/defconfig index 3cfa8ca26738,d53a1b00ad82.. --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@@ -355,8 -333,8 +356,14 @@@ CONFIG_GPIO_XGENE_SB= CONFIG_GPIO_PCA953X=y CONFIG_GPIO_PCA953X_IRQ=y CONFIG_GPIO_MAX77620=y ++<<< HEAD +CONFIG_POWER_AVS=y +CONFIG_ROCKCHIP_IODOMAIN=y ++||| merged common ancestors ++=== + CONFIG_POWER_AVS=y + CONFIG_QCOM_CPR=y ++>>> tracking-qcomlt-8016-dvfs CONFIG_POWER_RESET_MSM=y CONFIG_POWER_RESET_XGENE=y CONFIG_POWER_RESET_SYSCON=y that explains it.. it was simpler than what I thought.. thanks!
Re: [PATCH 14/29] t: drop subshell with missing &&-chain in favor of simpler construct
Eric Sunshine writes: > These tests employ a noisy subshell (with missing &&-chain) to feed > input into Git commands: > > (echo a; echo b; echo c) | git some-command ... > > Drop the subshell in favor of a simple 'printf': > > printf "%s\n" a b c | git some-command ... That's called test_write_lines, I think.
Re: [PATCH v2] filter-branch: skip commits present on --state-branch
On Mon, 2018-06-25 at 21:07 -0700, Michael Barabanov wrote: > The commits in state:filter.map have already been processed, so don't > filter them again. This makes incremental git filter-branch much > faster. > > Also add tests for --state-branch option. > > Signed-off-by: Michael Barabanov Acked-by: Ian Campbell > --- > git-filter-branch.sh | 1 + > t/t7003-filter-branch.sh | 15 +++ > 2 files changed, 16 insertions(+) > > diff --git a/git-filter-branch.sh b/git-filter-branch.sh > index ccceaf19a..5c5afa2b9 100755 > --- a/git-filter-branch.sh > +++ b/git-filter-branch.sh > @@ -372,6 +372,7 @@ while read commit parents; do > git_filter_branch__commit_count=$(($git_filter_branch__commi > t_count+1)) > > report_progress > + test -f "$workdir"/../map/$commit && continue > > case "$filter_subdir" in > "") > diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh > index ec4b160dd..e23de7d0b 100755 > --- a/t/t7003-filter-branch.sh > +++ b/t/t7003-filter-branch.sh > @@ -107,6 +107,21 @@ test_expect_success 'test that the directory was > renamed' ' > test dir/D = "$(cat diroh/D.t)" > ' > > +V=$(git rev-parse HEAD) > + > +test_expect_success 'populate --state-branch' ' > + git filter-branch --state-branch state -f --tree-filter > "touch file || :" HEAD > +' > + > +W=$(git rev-parse HEAD) > + > +test_expect_success 'using --state-branch to skip already rewritten > commits' ' > + test_when_finished git reset --hard $V && > + git reset --hard $V && > + git filter-branch --state-branch state -f --tree-filter > "touch file || :" HEAD && > + test_cmp_rev $W HEAD > +' > + > git tag oldD HEAD~4 > test_expect_success 'rewrite one branch, keeping a side branch' ' > git branch modD oldD &&
Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Eric Sunshine writes: > The --chain-lint option detects broken &&-chains by forcing the test to > exit early (as the very first step) with a sentinel value. If that > sentinel is the test's overall exit code, then the &&-chain is intact; > if not, then the chain is broken. Unfortunately, this detection does not > extend to &&-chains within subshells even when the subshell itself is > properly linked into the outer &&-chain. > > Address this shortcoming by eliminating the subshell during the > "linting" phase and incorporating its body directly into the surrounding > &&-chain. To keep this transformation cheap, no attempt is made at > properly parsing shell code. Instead, the manipulations are purely > textual. For example: > > statement1 && > ( > statement2 && > statement3 > ) && > statement4 > > is transformed to: > > statement1 && > statement2 && > statement3 && > statement4 so, with --chain-lint, we would transform this mkdir -p a/b/c && ( cd a/b/c rm -fr ../../* ) && statement 4 into this sequence (exit $sentinel) && mkdir -p a/b/c && cd a/b/c rm -fr ../../* && statement 4 and then rely on the non-zero exit to cancel all the remainder? We didn't create nor cd to the t/trash$num/a/b/c thanks to the && chain, and end up running rm -fr ../../* from inside t/trash$num? Hm
Re: git rerere and diff3
Nicolas Dechesne writes: > i have noticed that merge.conflictstyle has an impact on the rerere > resolution. looking briefly at the source code, it seems that git > tries to discard the common ancestor diff3 bits, but what I am seeing > is that if i do the following then it fails: > > 1. from a clean rr-cache state, with merge.conflictsytle=diff3, git > merge , resolve the conflicts, then commit > 2. undo the previous merge, remove merge.conflictstyle=diff3 (disable > diff3) and merge the *same* branch, then rerere won't fix the > conflicts It is possible that the conflict left when making the same merge are actually different when using these two conflict styles. IOW, if the merge produces <<< side A ||| common === side B >>> when diff3 style is chosen, but if the same merge results in <<< side A' === side B' >>> where side A' is not identical to side A (or B' and B are not identical), then we will fail to find the previously recorded resolution.
Re: [PATCH] fetch-pack: support negotiation tip whitelist
Jonathan Tan writes: > During negotiation, fetch-pack eventually reports as "have" lines all > commits reachable from all refs. Allow the user to restrict the commits > sent in this way by providing a whitelist of tips; only the tips > themselves and their ancestors will be sent. > > This feature is only supported for protocols that support connect or > stateless-connect (such as HTTP with protocol v2). > > This will speed up negotiation when the repository has multiple > relatively independent branches (for example, when a repository > interacts with multiple repositories, such as with linux-next [1] and > torvalds/linux [2]), and the user knows which local branch is likely to > have commits in common with the upstream branch they are fetching. > > [1] https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/ > [2] https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux/ > > Signed-off-by: Jonathan Tan > --- > This is based on jt/fetch-pack-negotiator, but if that branch is > undesirable for whatever reason, I can port this to master. > --- > builtin/fetch.c| 21 ++ > fetch-pack.c | 19 ++-- > fetch-pack.h | 7 ++ > t/t5510-fetch.sh | 55 ++ > transport-helper.c | 3 +++ > transport.c| 1 + > transport.h| 10 + > 7 files changed, 114 insertions(+), 2 deletions(-) What's the plan to expose this "feature" to end-users? There is no end-user facing documentation added by this patch, and in-code comments only talk about what (mechanical) effect the option has, but not when a user may want to use the feature, or how the user would best decide the set of commits to pass to this new option. Would something like this git fetch $(git for-each-ref \ --format=--nego-tip="%(objectname)" \ refs/remotes/linux-next/) \ linux-next be an expected typical way to pull from one remote, exposing only the tips of refs we got from that remote and not the ones we obtained from other places?
Re: [BUG] url schemes should be case-insensitive
On Tue, Jun 26, 2018 at 10:09:58AM -0700, Junio C Hamano wrote:
> > It may also interact in a funny way with our allowed-protocol code, if
> > "SSH" gets a pass as "ssh" under the default config, but actually runs
> > the otherwise-disallowed git-remote-SSH (though one would _hope_ if you
> > have such a git-remote-SSH that it behaves just like an ssh remote).
>
> True. I did not offhand recall how protocol whitelist matches the
> protocol name with config, but transport.c::get_protocol_config()
> seems to say that the part of "protocol..allow" is case
> sensitive, and we match known-safe (and known-unsafe "ext::")
> protocols with strcmp() not strcasecmp(). We need to figure out the
> implications of allowing SSH:// not to error out but pretending as
> if it were ssh:// on those who have protocol.ssh.allow defined.
That function is actually a little tricky, because we feed it mostly
from string literals (so we end up in the ssh code path, and then feed
it "ssh"). But I think for remote-helpers we feed it literally from the
URL we got fed.
So yeah, we would not want to allow EXT::"rm -rf /" to slip past the
known-unsafe match. Any normalization should happen before then
(probably right in transport_helper_init).
Come to think of it, that's already sort-of an issue now. If you have a
case-insensitive filesystem, then EXT:: is going to pass this check, but
still run git-remote-ext. We're saved there somewhat by the fact that
the default is to reject unknown helpers in submodules (otherwise, we'd
have that horrible submodule bug all over again).
That goes beyond just cases, too. On HFS+ I wonder if I could ask for
"\u{0200}ext::" and run git-remote-ext.
> > I think it doesn't help much if the user does not know what a remote
> > helper is, or why Git is looking for one.
>
> True.
>
> $ git clone SSH://example.com/repo.git
> fatal: unable to handle URL that begins with SSH://
>
> would be clear enough, perhaps? At least this line of change is a
> small first step that would improve the situation without potential
> to break anybody who has been abusing the case sensitivity loophole.
Yeah, certainly the advice is orthogonal to any behavior changes. The
original report complained of:
$ git clone SSH://...
fatal: 'remote-SSH' is not a git command. See 'git --help'.
but since 6b02de3b9d in 2010 we say:
fatal: Unable to find remote helper for 'SSH'
So I actually wonder if there is something else going on. I find it hard
to believe the OP is using something older than Git v1.7.0. They did
appear to be on Windows, though. Is it possible our ENOENT detection
from start_command() is not accurate on Windows?
-Peff
Re: [PATCH v4 3/9] t3422: new testcases for checking when incompatible options passed
Elijah Newren writes:
> +# Rebase has lots of useful options like --whitepsace=fix, which are
> +# actually all built in terms of flags to git-am. Since neither
> +# --merge nor --interactive (nor any options that imply those two) use
> +# git-am, using them together will result in flags like --whitespace=fix
> +# being ignored. Make sure rebase warns the user and aborts instead.
> +#
> +
> +test_rebase_am_only () {
> + opt=$1
> + shift
> + test_expect_failure "$opt incompatible with --merge" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --merge A
> + "
> +
> + test_expect_failure "$opt incompatible with --strategy=ours" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --strategy=ours A
> + "
> +
> + test_expect_failure "$opt incompatible with --strategy-option=ours" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --strategy=ours A
This line is broken and it is carried over to later patches. It
needs to be -Xours (or --strategy-option=ours, if we really want ot
be verbose).
> + "
> +
> + test_expect_failure "$opt incompatible with --interactive" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --interactive A
> + "
> +
> + test_expect_failure "$opt incompatible with --exec" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --exec 'true' A
> + "
> +
> +}
> +
> +test_rebase_am_only --whitespace=fix
> +test_rebase_am_only --ignore-whitespace
> +test_rebase_am_only --committer-date-is-author-date
> +test_rebase_am_only -C4
I was hesitant to hardcode what I perceive as limitations of non-am
rebase implementations with a test like this, but once somebody
fixes "rebase -i" for example to be capable of --whitespace=fix for
example, then we can just drop one line from the above four (and
write other tests for "rebase -i --whitespace=fix"). The
test_rebase_am_only is to help us make sure what is (still) not
supported by non-am rebases gets diagnosed as an error.
So my worry is totally unfounded, which is good.
> +test_expect_success '--preserve-merges incompatible with --signoff' '
> + git checkout B^0 &&
> + test_must_fail git rebase --preserve-merges --signoff A
> +'
> +
> +test_expect_failure '--preserve-merges incompatible with --rebase-merges' '
> + git checkout B^0 &&
> + test_must_fail git rebase --preserve-merges --rebase-merges A
> +'
> +
> +test_expect_failure '--rebase-merges incompatible with --strategy' '
> + git checkout B^0 &&
> + test_must_fail git rebase --rebase-merges -s resolve A
> +'
> +
> +test_expect_failure '--rebase-merges incompatible with --strategy-option' '
> + git checkout B^0 &&
> + test_must_fail git rebase --rebase-merges -Xignore-space-change A
> +'
> +
> +test_done
Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually
On Tue, Jun 26, 2018 at 2:06 PM Johannes Sixt wrote: > Am 26.06.2018 um 11:21 schrieb Eric Sunshine: > >> On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine > >> wrote: > >>> + p4 help client && > >>> + test_must_fail p4 help nosuchcommand > > [...] So, despite > > the (somewhat) misleading test title, this test doesn't care about the > > exact error code but rather cares only that "p4 help nosuchcommand" > > errors out, period. Hence, test_must_fail() again agrees with the > > spirit of the test. > > test_must_fail ensures that only "proper" failures are diagnosed as > expected; failures due to signals such as SEGV are not expected failures. > > In the test suite we expect all programs that are not our "git" to work > correctly; in particular, that they do not crash on anything that we ask > them to operate on. Under this assumption, the protection given by > test_must_fail is not needed. > > Hence, these lines should actually be > > p4 help client && > ! p4 help nosuchcommand Thanks for the comment; you're right, of course. I'll certainly make this change if I have to re-roll for some other reason, but do you feel that this itself is worth a re-roll?
Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually
Am 26.06.2018 um 11:21 schrieb Eric Sunshine: On Tue, Jun 26, 2018 at 4:58 AM Elijah Newren wrote: On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine wrote: + p4 help client && + test_must_fail p4 help nosuchcommand same question? Same answer. Not shown in this patch, but just above the context lines you will find this comment in the file: # We rely on this behavior to detect for p4 move availability. which means that the test is really interested in being able to reliably detect if a sub-command is or is not available. So, despite the (somewhat) misleading test title, this test doesn't care about the exact error code but rather cares only that "p4 help nosuchcommand" errors out, period. Hence, test_must_fail() again agrees with the spirit of the test. test_must_fail ensures that only "proper" failures are diagnosed as expected; failures due to signals such as SEGV are not expected failures. In the test suite we expect all programs that are not our "git" to work correctly; in particular, that they do not crash on anything that we ask them to operate on. Under this assumption, the protection given by test_must_fail is not needed. Hence, these lines should actually be p4 help client && ! p4 help nosuchcommand -- Hannes
[PATCH] rebase -i: Fix white space in comments
Fix a trivial white-space issue introduced by commit d48f97aa8
("rebase: reindent function git_rebase__interactive", 2018-03-23). This
affected the instructional comments displayed in the editor during an
interactive rebase.
Signed-off-by: dana
---
Sorry if i've done any of this wrong; i've never used this work-flow
before. In any case, if it's not immediately obvious, this is the issue
i mean to fix:
BEFORE (2.17.1):
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
# Note that empty commits are commented out
AFTER (2.18.0):
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
#
# Note that empty commits are commented out
The 2.18.0 version is particularly irritating because many editors
highlight the trailing tab in the penultimate line as a white-space
error.
Aside: It's not a new thing, but i've always felt like that last line
should end in a full stop. Maybe i'll send a patch for that too.
Cheers,
dana
git-rebase--interactive.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 299ded213..a31af6d4c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -222,9 +222,9 @@ $comment_char $(eval_ngettext \
EOF
append_todo_help
gettext "
- However, if you remove everything, the rebase will be aborted.
+However, if you remove everything, the rebase will be aborted.
- " | git stripspace --comment-lines >>"$todo"
+" | git stripspace --comment-lines >>"$todo"
if test -z "$keep_empty"
then
--
2.18.0
Re: [PATCH] fetch-pack: support negotiation tip whitelist
Hi,
Jonathan Tan wrote:
> During negotiation, fetch-pack eventually reports as "have" lines all
> commits reachable from all refs. Allow the user to restrict the commits
> sent in this way by providing a whitelist of tips; only the tips
> themselves and their ancestors will be sent.
>
> This feature is only supported for protocols that support connect or
> stateless-connect (such as HTTP with protocol v2).
>
> This will speed up negotiation when the repository has multiple
> relatively independent branches (for example, when a repository
> interacts with multiple repositories, such as with linux-next [1] and
> torvalds/linux [2]), and the user knows which local branch is likely to
> have commits in common with the upstream branch they are fetching.
>
> [1] https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/
> [2] https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux/
>
> Signed-off-by: Jonathan Tan
> ---
Very neat. Thanks to Greg Thelen and Michel Lespinasse (cc-ed) for
this suggestion.
> This is based on jt/fetch-pack-negotiator, but if that branch is
> undesirable for whatever reason, I can port this to master.
>
> builtin/fetch.c| 21 ++
> fetch-pack.c | 19 ++--
> fetch-pack.h | 7 ++
> t/t5510-fetch.sh | 55 ++
> transport-helper.c | 3 +++
> transport.c| 1 +
> transport.h| 10 +
> 7 files changed, 114 insertions(+), 2 deletions(-)
Small nit: could this be documented in Documentation/fetch.txt as well?
Thanks,
Jonathan
Patch left unsnipped for reference.
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ea5b9669a..12daec0f3 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -63,6 +63,7 @@ static int shown_url = 0;
> static struct refspec refmap = REFSPEC_INIT_FETCH;
> static struct list_objects_filter_options filter_options;
> static struct string_list server_options = STRING_LIST_INIT_DUP;
> +static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
>
> static int git_fetch_config(const char *k, const char *v, void *cb)
> {
> @@ -174,6 +175,8 @@ static struct option builtin_fetch_options[] = {
> TRANSPORT_FAMILY_IPV4),
> OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"),
> TRANSPORT_FAMILY_IPV6),
> + OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
> + N_("report that we have only objects reachable from
> this object")),
> OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
> OPT_END()
> };
> @@ -1075,6 +1078,24 @@ static struct transport *prepare_transport(struct
> remote *remote, int deepen)
> filter_options.filter_spec);
> set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
> }
> + if (negotiation_tip.nr) {
> + struct oid_array *oids;
> + if (transport->smart_options) {
> + int i;
> + oids = xcalloc(1, sizeof(*oids));
> + for (i = 0; i < negotiation_tip.nr; i++) {
> + struct object_id oid;
> + if (get_oid(negotiation_tip.items[i].string,
> + &oid))
> + die("%s is not a valid object",
> + negotiation_tip.items[i].string);
> + oid_array_append(oids, &oid);
> + }
> + transport->smart_options->negotiation_tips = oids;
> + } else {
> + warning("Ignoring --negotiation-tip because the
> protocol does not support it.");
> + }
> + }
> return transport;
> }
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index ba12085c4..c66bd49bd 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -213,6 +213,21 @@ static int next_flush(int stateless_rpc, int count)
> return count;
> }
>
> +static void mark_tips(struct fetch_negotiator *negotiator,
> + const struct oid_array *negotiation_tips)
> +{
> + int i;
> + if (!negotiation_tips) {
> + for_each_ref(rev_list_insert_ref_oid, negotiator);
> + return;
> + }
> +
> + for (i = 0; i < negotiation_tips->nr; i++)
> + rev_list_insert_ref(negotiator, NULL,
> + &negotiation_tips->oid[i]);
> + return;
> +}
> +
> static int find_common(struct fetch_negotiator *negotiator,
> struct fetch_pack_args *args,
> int fd[2], struct object_id *result_oid,
> @@ -230,7 +245,7 @@ static int find_common(struct fetch_negotiator
> *negotiator,
> if (args->stateless_rpc && multi_ack == 1)
> die(_("--stateless-rpc requires multi_ack_detailed"));
>
> - for_each_ref(rev_lis
Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
Alban Gruin writes: I do not think "base_commit" is a good name, either, though. When I hear 'base' in the context of 'rebase', I would imagine that the speaker is talking about the bottom of the range of the commits to be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays commits BASE..BRANCH on top of ONTO and then points BRANCH at the result), not the top of the range or the branch these commits are taken from. >>> ... > Now I really don’t know how to call this function. > checkout_top_of_range(), perhaps? If this is a straight rewrite of setup_reflog_action, i.e. the division of labor between its caller and this function is such that the caller blindly calls it without even checking if it got the optional "branch to be rebased" argument and this function is responsible to decide if the preparatory checkout of the named branch is necessary, then any name that does not even hint that checkout is done conditionally would not work well. How about callilng it "prepare_branch_to_be_rebased()"? This function, at least the original setup_reflog_action, responds to git rebase [--onto ONTO] UPSTREAM by doing nothing (i.e. the result of preparation is to do nothing because we are rebasing the commits between UPSTREAM and currently checked out state on top of ONTO, or UPSTREAM if ONTO is not given) and it responds to git rebase [--onto ONTO] UPSTREAM BRANCH by checking out BRANCH (most importantly, when given a concrete branch name, it checks the branch out, and does not detach at the commit at the tip of the branch), because that is the first preparatory step to rebase the BRANCH.
Re: [GSoC][PATCH v4 2/3] rebase -i: rewrite checkout_onto() in C
Alban Gruin writes:
> This rewrites checkout_onto() from shell to C. The new version is called
> detach_onto(), given its role.
The name, given its role, may be good, but is the implementtaion
robust enough to fulfill the promise its name gives?
> git rebase--helper --check-todo-list || {
> ret=$?
> - checkout_onto
> + git rebase--helper --detach-onto "$onto_name" "$onto" \
> + "$orig_head" ${verbose:+--verbose}
Here, $onto_name is what the end-user gave us (e.g. it is
"master..." in "git rebase --onto=master... base"), while $onto is a
40-hex object name of the commit. $orig_head is also a 40-hex
object name.
And this call shows how the above shell scriptlet calls into the
detach_onto() thing ...
> + if (command == DETACH_ONTO && argc == 4)
> + return !!detach_onto(&opts, argv[1], argv[2], argv[3], verbose);
... which is defined like so:
> +int detach_onto(struct replay_opts *opts,
> + const char *onto_name, const char *onto,
> + const char *orig_head, unsigned verbose)
> +{
> + struct object_id oid;
> + const char *action = reflog_message(opts, "start", "checkout %s",
> onto_name);
> +
> + if (get_oid(orig_head, &oid))
> + return error(_("%s: not a valid OID"), orig_head);
Which means that this can be more strict to use get_oid_hex() to
catch possible mistakes in the caller.
> + if (run_git_checkout(opts, onto, verbose, action)) {
And this could be a bit problematic, as we can see below how the
"checkout" thing does not guarantee "detaching" at all ...
> + apply_autostash(opts);
> + sequencer_remove_state(opts);
> + return error(_("could not detach HEAD"));
> + }
> +
> + return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0,
> UPDATE_REFS_MSG_ON_ERR);
> +}
> +
... which can be seen here ...
> +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> + int verbose, const char *action)
> +{
> + struct child_process cmd = CHILD_PROCESS_INIT;
> +
> + cmd.git_cmd = 1;
> +
> + argv_array_push(&cmd.args, "checkout");
> + argv_array_push(&cmd.args, commit);
> + argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> +
> + if (verbose)
> + return run_command(&cmd);
> + else
> + return run_command_silent_on_success(&cmd);
> +}
This drives the external command "git checkout" with _any_ string
the caller passes in "commit". If the variable happens to have
'master', for example, it would be "git checkout master" and if you
have a branch with that name, it will not detach but check out the
branch to build on it. It is a caller's responsibility to give a
suitable "commit" if it wants to use this helper to detach.
So perhaps the caller of this function in detach_onto() should pass
"%s^0" or even do something like
struct object_id onto_oid;
char onto_hex[GIT_MAX_HEXSZ + 1];
if (get_oid(onto, &onto_oid) || oid_to_hex_r(onto_hex, &onto_oid))
return error(...);
if (run_git_checkout(opts, onto_hex, verbose, action)) {
...
to ensure that it keeps the promise its name gives.
I can hear "Oh, but it is a bug in the caller to give anything that
won't result in detaching in 'onto'" but that is not a valid excuse,
given that this _public_ function is called "detach_onto". Making
sure detachment happens is its responsibility, not its callers'.
Or we could do a cop-out alternative of commenting the function in *.h
file to say "onto must be given as 40-hex", with a code to make sure
the caller really gave us a 40-hex and not a branch name. That is a
less ideal but probably acceptable alternative.
> static const char rescheduled_advice[] =
> N_("Could not execute the todo command\n"
> "\n"
> diff --git a/sequencer.h b/sequencer.h
> index 35730b13e..9f0ac5e75 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -100,6 +100,10 @@ int update_head_with_reflog(const struct commit
> *old_head,
> void commit_post_rewrite(const struct commit *current_head,
>const struct object_id *new_head);
>
> +int detach_onto(struct replay_opts *opts,
> + const char *onto_name, const char *onto,
> + const char *orig_head, unsigned verbose);
> +
> #define SUMMARY_INITIAL_COMMIT (1 << 0)
> #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
> void print_commit_summary(const char *prefix, const struct object_id *oid,
Re: [BUG] url schemes should be case-insensitive
Jeff King writes: >> > We seem to match url schemes case-sensitively: >> > >> > $ git clone SSH://example.com/repo.git >> > Cloning into 'repo'... >> > fatal: Unable to find remote helper for 'SSH' >> > >> > whereas rfc3986 is clear that the scheme portion is case-insensitive. >> > We probably ought to match at least our internal ones with strcasecmp. >> >> That may break if somebody at DevToolGroup@$BIGCOMPANY got cute and >> named their custom remote helper SSH:// that builds on top of the >> normal ssh:// protocol with something extra and gave it to their >> developers (and they named the http counterpart that has the same >> extra HTTP://, of course). > > True, though I am on the fence whether that is a property worth > maintaining. AFAIK it was not planned and is just a "this is how it > happened to work" case that is (IMHO) doing the wrong thing. FWIW, I fully agree with the assessment; sorry for not saying that together with the devil's advocate comment to save a round-tip. > It may also interact in a funny way with our allowed-protocol code, if > "SSH" gets a pass as "ssh" under the default config, but actually runs > the otherwise-disallowed git-remote-SSH (though one would _hope_ if you > have such a git-remote-SSH that it behaves just like an ssh remote). True. I did not offhand recall how protocol whitelist matches the protocol name with config, but transport.c::get_protocol_config() seems to say that the part of "protocol..allow" is case sensitive, and we match known-safe (and known-unsafe "ext::") protocols with strcmp() not strcasecmp(). We need to figure out the implications of allowing SSH:// not to error out but pretending as if it were ssh:// on those who have protocol.ssh.allow defined. >> > We could probably also give an advise() message in the above output, >> > suggesting that the problem is likely one of: >> > >> > 1. They misspelled the scheme. >> > >> > 2. They need to install the appropriate helper. >> > >> > This may be a good topic for somebody looking for low-hanging fruit to >> > get involved in development (I'd maybe call it a #leftoverbits, but >> > since I didn't start on it, I'm not sure if it counts as "left over" ;)). >> [..] >> It may probably be a good idea to do an advice, but I'd think >> "Untable to find remote helper for 'SSH'" may be clear enough. If >> anything, perhaps saying "remote helper for 'SSH' protocol" would >> make it even clear? I dunno. > > I think it doesn't help much if the user does not know what a remote > helper is, or why Git is looking for one. True. $ git clone SSH://example.com/repo.git fatal: unable to handle URL that begins with SSH:// would be clear enough, perhaps? At least this line of change is a small first step that would improve the situation without potential to break anybody who has been abusing the case sensitivity loophole.
Re: [PATCH 10/29] t9001: fix broken "invoke hook" test
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh > index e80eacbb1b..776769fe0d 100755 > --- a/t/t9001-send-email.sh > +++ b/t/t9001-send-email.sh > @@ -1966,11 +1966,11 @@ test_expect_success $PREREQ 'invoke hook' ' > > # Verify error message when a patch is rejected by the hook > sed -e "s/add master/x/" ../0001-add-master.patch > >../another.patch && > - git send-email \ > + test_must_fail git send-email \ > --from="Example " \ > --to=nob...@example.com \ > --smtp-server="$(pwd)/../fake.sendmail" \ > - ../another.patch 2>err > + ../another.patch 2>err && > test_i18ngrep "rejected by sendemail-validate hook" err Thanks for catching this. Indeed, "git send-email" is supposed to fail because the validate hook greps for the string "add master", which does not exist in the e-mail to be sent. (Above this is a test that shows that the same validate hook succeeds if the e-mail contains "add master".) This looks correct to me.
Re: [PATCH v2 0/6] Restrict the usage of config_from_gitmodules to submodule-config
On 06/26, Antonio Ospite wrote: > Hi, > > this is version 2 of the series from > https://public-inbox.org/git/20180622162656.19338-1-...@ao2.it/ > > The .gitmodules file is not meant for arbitrary configuration, it should > be used only for submodules properties. > > Plus, arbitrary git configuration should not be distributed with the > repository, and .gitmodules might be a possible "vector" for that. > > The series tries to alleviate both these issues by moving the > 'config_from_gitmodules' function from config.[ch] to submodule-config.c > and making it private. > > This should discourage future code from using the function with > arbitrary config callbacks which might turn .gitmodules into a mechanism > to load arbitrary configuration stored in the repository. > > Backward compatibility exceptions to the rules above are handled by > ad-hoc helpers. > > Finally (in patch 6) some duplication is removed by using > 'config_from_gitmodules' to load the submodules configuration in > 'repo_read_gitmodules'. > > Changes since v1: > * Remove an extra space before an arrow operator in patch 2 > * Fix a typo in the commit message of patch 3: s/fetchobjs/fetchjobs > * Add a note in the commit message of patch 6 about checking the > worktree before loading .gitmodules > * Drop patch 7, it was meant as a cleanup but resulted in parsing the > .gitmodules file twice Thanks for making these changes, this version looks good to me! -- Brandon Williams
Re: curious about wording in "man git-config", ENVIRONMENT
On Tue, 26 Jun 2018, Jeff King wrote: > On Tue, Jun 26, 2018 at 06:18:26AM -0400, Robert P. J. Day wrote: > > > > > ENVIRONMENT > > GIT_CONFIG > > Take the configuration from the given file instead of > > .git/config. Using the "--global" option forces this to > > ~/.gitconfig. Using the "--system" option forces this to > > $(prefix)/etc/gitconfig. > > > > is the phrase "forces this to" really what you want to use here? > > maybe i misunderstand what this option does, doesn't it simply mean > > that it will use a different (specified) file from the default, > > depending on the context (local, global, system)? > > > > it just seems weird to say that the option "forces" the use of what > > are clearly the default files. thoughts? > > I agree it's weird. I think it's trying to mean "behaves as if it > was set to", but with the additional notion that the command-line > argument would take precedence over the environment (which is our > usual rule). But then we should just say those things explicitly. > > Just looking at mentions of GIT_CONFIG in that manpage and knowing > the history, I think: ... snip ... i'm just going to admit that i don't quite have the background to know how to submit a patch to tidy things up based on Jeff's analysis, so I'm going to leave this to someone higher up the food chain. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: [PATCH v3 0/7] grep.c: teach --column to 'git-grep(1)'
Taylor Blau writes: > On Mon, Jun 25, 2018 at 02:43:50PM -0400, Jeff King wrote: >> On Fri, Jun 22, 2018 at 10:49:26AM -0500, Taylor Blau wrote: >> > Since the last time, only a couple of things have changed at Peff's >> > suggestions in [1]. The changes are summarized here, and an inter-diff >> > is available below: >> > >> > - Change "%zu" to PRIuMAX (and an appropriate cast into uintmax_t). I >> > plan to send a follow-up patch to convert this back to "%zu" to see >> > how people feel about it, but I wanted to keep that out of the >> > present series in order to not hold things up. >> ... >> Jinxes aside, this interdiff looks good to me. > > Thanks; I hope that I haven't jinxed anything :-). > > I'm going to avoid sending the PRIuMAX -> "%zu" patch, since dscho > points out that it's not available on Windows [1]. OK, so what I queued on 'pu' seems to be ready to advance, which is good. Keeping topics in flight on 'pu', unable to convince myself that they are ready to advance to 'next', makes me feel uneasy and unhappy, and having to worry about one less such topic is a good news ;-)
Re: [PATCH 4/5] commit-graph: store graph in struct object_store
Jonathan Tan writes: > As for whether both these functions are necessary in the first place, I > think they are. I do not mind their existence. I was wondering if they can share more implementation; such a design would need s/the_commit_graph/the_repo->objstore->commit_graph/ only once as a side effect.
[GSoC][PATCH v3 0/2] rebase -i: rewrite the edit-todo functionality in C
This patch rewrites the edit-todo functionality from shell to C. This is part of the effort to rewrite interactive rebase in C. This patch is based on the fourth iteration of my series rewriting append_todo_help() in C. Changes since v2: - Moving edit_todo() from sequencer.c to interactive-rebase.c. Alban Gruin (2): editor: add a function to launch the sequence editor rebase-interactive: rewrite the edit-todo functionality in C builtin/rebase--helper.c | 13 - cache.h| 1 + editor.c | 27 +-- git-rebase--interactive.sh | 11 +-- rebase-interactive.c | 31 +++ rebase-interactive.h | 1 + strbuf.h | 2 ++ 7 files changed, 69 insertions(+), 17 deletions(-) -- 2.18.0
[GSoC][PATCH v3 1/2] editor: add a function to launch the sequence editor
As part of the rewrite of interactive rebase, the sequencer will need to
open the sequence editor to allow the user to edit the todo list.
Instead of duplicating the existing launch_editor() function, this
refactors it to a new function, launch_specified_editor(), which takes
the editor as a parameter, in addition to the path, the buffer and the
environment variables. launch_sequence_editor() is then added to launch
the sequence editor.
Signed-off-by: Alban Gruin
---
cache.h | 1 +
editor.c | 27 +--
strbuf.h | 2 ++
3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 8b447652a..d70ae49ca 100644
--- a/cache.h
+++ b/cache.h
@@ -1409,6 +1409,7 @@ extern const char *fmt_name(const char *name, const char
*email);
extern const char *ident_default_name(void);
extern const char *ident_default_email(void);
extern const char *git_editor(void);
+extern const char *git_sequence_editor(void);
extern const char *git_pager(int stdout_is_tty);
extern int is_terminal_dumb(void);
extern int git_ident_config(const char *, const char *, void *);
diff --git a/editor.c b/editor.c
index 9a9b4e12d..c985eee1f 100644
--- a/editor.c
+++ b/editor.c
@@ -1,4 +1,5 @@
#include "cache.h"
+#include "config.h"
#include "strbuf.h"
#include "run-command.h"
#include "sigchain.h"
@@ -34,10 +35,21 @@ const char *git_editor(void)
return editor;
}
-int launch_editor(const char *path, struct strbuf *buffer, const char *const
*env)
+const char *git_sequence_editor(void)
{
- const char *editor = git_editor();
+ const char *editor = getenv("GIT_SEQUENCE_EDITOR");
+
+ if (!editor)
+ git_config_get_string_const("sequence.editor", &editor);
+ if (!editor)
+ editor = git_editor();
+ return editor;
+}
+
+static int launch_specified_editor(const char *editor, const char *path,
+ struct strbuf *buffer, const char *const
*env)
+{
if (!editor)
return error("Terminal is dumb, but EDITOR unset");
@@ -95,3 +107,14 @@ int launch_editor(const char *path, struct strbuf *buffer,
const char *const *en
return error_errno("could not read file '%s'", path);
return 0;
}
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const
*env)
+{
+ return launch_specified_editor(git_editor(), path, buffer, env);
+}
+
+int launch_sequence_editor(const char *path, struct strbuf *buffer,
+ const char *const *env)
+{
+ return launch_specified_editor(git_sequence_editor(), path, buffer,
env);
+}
diff --git a/strbuf.h b/strbuf.h
index 60a35aef1..66da9822f 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -575,6 +575,8 @@ extern void strbuf_add_unique_abbrev(struct strbuf *sb,
* file's contents are not read into the buffer upon completion.
*/
extern int launch_editor(const char *path, struct strbuf *buffer, const char
*const *env);
+extern int launch_sequence_editor(const char *path, struct strbuf *buffer,
+ const char *const *env);
extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char
*buf, size_t size);
--
2.18.0
[GSoC][PATCH v3 2/2] rebase-interactive: rewrite the edit-todo functionality in C
This rewrites the edit-todo functionality from shell to C.
To achieve that, a new command mode, `edit-todo`, is added, and the
`write-edit-todo` flag is removed, as the shell script does not need to
write the edit todo help message to the todo list anymore.
The shell version is then stripped in favour of a call to the helper.
Signed-off-by: Alban Gruin
---
builtin/rebase--helper.c | 13 -
git-rebase--interactive.sh | 11 +--
rebase-interactive.c | 31 +++
rebase-interactive.h | 1 +
4 files changed, 41 insertions(+), 15 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 05e73e71d..731a64971 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo
= 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -28,8 +28,6 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge
commits")),
OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
N_("keep original branch points of cousins")),
- OPT_BOOL(0, "write-edit-todo", &write_edit_todo,
-N_("append the edit-todo message to the todo-list")),
OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -50,6 +48,9 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("insert exec commands in todo list"), ADD_EXEC),
OPT_CMDMODE(0, "append-todo-help", &command,
N_("insert the help in the todo list"),
APPEND_TODO_HELP),
+ OPT_CMDMODE(0, "edit-todo", &command,
+ N_("edit the todo list during an interactive
rebase"),
+ EDIT_TODO),
OPT_END()
};
@@ -90,6 +91,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
if (command == APPEND_TODO_HELP && argc == 1)
- return !!append_todo_help(write_edit_todo, keep_empty);
+ return !!append_todo_help(0, keep_empty);
+ if (command == EDIT_TODO && argc == 1)
+ return !!edit_todo_list(flags);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 94c23a7af..2defe607f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -108,16 +108,7 @@ initiate_action () {
--continue
;;
edit-todo)
- git stripspace --strip-comments <"$todo" >"$todo".new
- mv -f "$todo".new "$todo"
- collapse_todo_ids
- git rebase--helper --append-todo-help --write-edit-todo
-
- git_sequence_editor "$todo" ||
- die "$(gettext "Could not execute editor")"
- expand_todo_ids
-
- exit
+ exec git rebase--helper --edit-todo
;;
show-current-patch)
exec git show REBASE_HEAD --
diff --git a/rebase-interactive.c b/rebase-interactive.c
index c79c819b9..ace8e095b 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -66,3 +66,34 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
return ret;
}
+
+int edit_todo_list(unsigned flags)
+{
+ struct strbuf buf = STRBUF_INIT;
+ const char *todo_file = rebase_path_todo();
+ FILE *todo;
+
+ if (strbuf_read_file(&buf, todo_file, 0) < 0)
+ return error_errno(_("could not read '%s'."), todo_file);
+
+ strbuf_stripspace(&buf, 1);
+ todo = fopen_or_warn(todo_file, "w");
+ if (!todo) {
+ strbuf_release(&buf);
+ return 1;
+ }
+
+ strbuf_write(&buf, todo);
+ fclose(todo);
+ strbuf_release(&buf);
+
+ transform_todos(flags | TODO_LIST_SHORTEN_IDS);
+ ap
[GSoC][PATCH v4 2/2] rebase--interactive: rewrite append_todo_help() in C
This rewrites append_todo_help() from shell to C. It also incorporates
some parts of initiate_action() and complete_action() that also write
help texts to the todo file.
This also introduces the source file rebase-interactive.c. This file
will contain functions necessary for interactive rebase that are too
specific for the sequencer, and is part of libgit.a.
Two flags are added to rebase--helper.c: one to call
append_todo_help() (`--append-todo-help`), and another one to tell
append_todo_help() to write the help text suited for the edit-todo
mode (`--write-edit-todo`).
Finally, append_todo_help() is removed from git-rebase--interactive.sh
to use `rebase--helper --append-todo-help` instead.
Signed-off-by: Alban Gruin
---
Makefile | 1 +
builtin/rebase--helper.c | 11 --
git-rebase--interactive.sh | 52 ++---
rebase-interactive.c | 68 ++
rebase-interactive.h | 6
5 files changed, 86 insertions(+), 52 deletions(-)
create mode 100644 rebase-interactive.c
create mode 100644 rebase-interactive.h
diff --git a/Makefile b/Makefile
index 0cb6590f2..a281139ef 100644
--- a/Makefile
+++ b/Makefile
@@ -922,6 +922,7 @@ LIB_OBJS += protocol.o
LIB_OBJS += quote.o
LIB_OBJS += reachable.o
LIB_OBJS += read-cache.o
+LIB_OBJS += rebase-interactive.o
LIB_OBJS += reflog-walk.o
LIB_OBJS += refs.o
LIB_OBJS += refs/files-backend.o
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc..05e73e71d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -3,6 +3,7 @@
#include "config.h"
#include "parse-options.h"
#include "sequencer.h"
+#include "rebase-interactive.h"
static const char * const builtin_rebase_helper_usage[] = {
N_("git rebase--helper []"),
@@ -12,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo
= 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC
+ ADD_EXEC, APPEND_TODO_HELP
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +28,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge
commits")),
OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
N_("keep original branch points of cousins")),
+ OPT_BOOL(0, "write-edit-todo", &write_edit_todo,
+N_("append the edit-todo message to the todo-list")),
OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -45,6 +48,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
OPT_CMDMODE(0, "add-exec-commands", &command,
N_("insert exec commands in todo list"), ADD_EXEC),
+ OPT_CMDMODE(0, "append-todo-help", &command,
+ N_("insert the help in the todo list"),
APPEND_TODO_HELP),
OPT_END()
};
@@ -84,5 +89,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!rearrange_squash();
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
+ if (command == APPEND_TODO_HELP && argc == 1)
+ return !!append_todo_help(write_edit_todo, keep_empty);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 299ded213..94c23a7af 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -39,38 +39,6 @@ comment_for_reflog () {
esac
}
-append_todo_help () {
- gettext "
-Commands:
-p, pick = use commit
-r, reword = use commit, but edit the commit message
-e, edit = use commit, but stop for amending
-s, squash = use commit, but meld into previous commit
-f, fixup = like \"squash\", but discard this commit's log message
-x, exec = run command (the rest of the line) using shell
-d, drop = remove commit
-l, label = label current HEAD with a name
-t, reset = reset HEAD to a label
-m, merge [-C | -c ] [# ]
-. create a merge commit using the original merge commit's
-. messa
[GSoC][PATCH v4 1/2] sequencer: make two functions and an enum from sequencer.c public
This makes rebase_path_todo(), get_missign_commit_check_level() and the
enum check_level accessible outside sequencer.c. This will be needed
for the rewrite of append_todo_help() from shell to C, as it will be in
a new library source file, rebase-interactive.c.
Signed-off-by: Alban Gruin
---
sequencer.c | 8 ++--
sequencer.h | 6 ++
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0a291c91f..881a4f7f4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -52,7 +52,7 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
* the lines are processed, they are removed from the front of this
* file and written to the tail of 'done'.
*/
-static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
+GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
/*
* The rebase command lines that have already been processed. A line
* is moved here when it is first handled, before any associated user
@@ -4223,11 +4223,7 @@ int transform_todos(unsigned flags)
return i;
}
-enum check_level {
- CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
-};
-
-static enum check_level get_missing_commit_check_level(void)
+enum check_level get_missing_commit_check_level(void)
{
const char *value;
diff --git a/sequencer.h b/sequencer.h
index c5787c6b5..08397b0d1 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -3,6 +3,7 @@
const char *git_path_commit_editmsg(void);
const char *git_path_seq_dir(void);
+const char *rebase_path_todo(void);
#define APPEND_SIGNOFF_DEDUP (1u << 0)
@@ -57,6 +58,10 @@ struct replay_opts {
};
#define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
+enum check_level {
+ CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
+};
+
/* Call this to setup defaults before parsing command line options */
void sequencer_init_config(struct replay_opts *opts);
int sequencer_pick_revisions(struct replay_opts *opts);
@@ -79,6 +84,7 @@ int sequencer_make_script(FILE *out, int argc, const char
**argv,
int sequencer_add_exec_commands(const char *command);
int transform_todos(unsigned flags);
+enum check_level get_missing_commit_check_level(void);
int check_todo_list(void);
int skip_unnecessary_picks(void);
int rearrange_squash(void);
--
2.18.0
[GSoC][PATCH v4 0/2] rebase -i: rewrite append_todo_help() in C
This patch rewrites append_todo_help() from shell to C. The C version covers a bit more than the old shell version. To achieve that, some parameters were added to rebase--helper. This also introduce a new source file, rebase-interactive.c. This is part of the effort to rewrite interactive rebase in C. This is based on next, as of 2018-06-26. Changes since v3: - Show an error message when append_todo_help() fails to edit the todo list. - Introducing rebase-interactive.c to contain functions necessary for interactive rebase. Alban Gruin (2): sequencer: make two functions and an enum from sequencer.c public rebase--interactive: rewrite append_todo_help() in C Makefile | 1 + builtin/rebase--helper.c | 11 -- git-rebase--interactive.sh | 52 ++--- rebase-interactive.c | 68 ++ rebase-interactive.h | 6 sequencer.c| 8 ++--- sequencer.h| 6 7 files changed, 94 insertions(+), 58 deletions(-) create mode 100644 rebase-interactive.c create mode 100644 rebase-interactive.h -- 2.18.0
Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect
Hi Dscho,
On Tue, Jun 26, 2018 at 4:10 PM, Johannes Schindelin
wrote:
>
> On Tue, 26 Jun 2018, Christian Couder wrote:
>
>> On Mon, Jun 25, 2018 at 7:33 PM, Junio C Hamano wrote:
>>
>> > I hate to say this, but the above looks like a typical
>> > unmaintainable mess.
>> >
>> > What happens when you or somebody else later needs to update the
>> > graph to be tested to add one more commit (or even more)? Would it
>> > be enough to add another "rev-parse" plus "dist=X" line in both
>> > expects? Or do we see a trap for combinatorial explosion that
>> > requires us to add new expect$N?
>>
>> What about the following then:
>>
>> test_dist_order () {
>> file="$1"
>> n="$2"
>> while read -r hash dist
>> do
>> d=$(echo "$dist" | sed -e "s/(dist=\(.*\))/\1/")
>> case "$d" in
>> ''|*[!0-9]*) return 1 ;;
>> *) ;;
>> esac
>> test "$d" -le "$n" || return 1
>> n="$d"
>> done <"$file"
>> }
>>
>> test_expect_success "--bisect-all --first-parent" '
>> cat >expect <> $(git rev-parse CC) (dist=2)
>> $(git rev-parse EX) (dist=1)
>> $(git rev-parse D) (dist=1)
>> $(git rev-parse FX) (dist=0)
>> EOF
>> sort expect >expect_sorted &&
>> git rev-list --bisect-all --first-parent FX ^A >actual &&
>> sort actual >actual_sorted &&
>> test_cmp expect_sorted actual_sorted &&
>> test_dist_order actual 2
>> '
>>
>> This feels overkill to me, but it should scale if we ever make more
>> complex tests.
>
> I *think* that you misunderstand Junio. At least when I read this:
>
>> $(git rev-parse CC) (dist=2)
>> $(git rev-parse EX) (dist=1)
>> $(git rev-parse D) (dist=1)
>> $(git rev-parse FX) (dist=0)
>
> I go: Huh?!?!?!?!?!
>
> What's CC? Is it Christian Couder? Creative Commons? Crudely Complex?
I agree that the name of the commit could be improved.
> The point, for me, is: if this test fails, at some stage in the future,
> for any reason, it will be a major pain to even dissect what the test is
> supposed to do.
I think the test is quite simple and there is an ascii picture, so it
is not so difficult to understand.
> This is no good. And you can do better. A lot better. You
> can write the test in a way that the head of a reader does not hurt, and
> at the same time it is still obvious what it does, and obvious that it
> does the right thing.
Obviousness is often not the same for everybody.
> One thing that makes the brain stumble for certain is when you deviate
> from the conventions, especially when it is for no good reason at all. In
> this case (and I am not sure why you, as a long-time contributor, did not
> spot this before public review):
Please note that I never committed myself (like most of us who are not
maintainer actually) to reviewing everything bisect related (or
everything that Tiago works on).
> - the titles of the test cases leave a lot of room for improvement,
>
> - the lines are too long,
>
> - the convention to end the `test_expect_success` line with an opening
> single quote is not heeded,
If you take a look at the beginning of the script you will see that
there are those problems there too.
I know that we should try to do better, but here I am mostly
interested in moving forward a feature that people have requested for
ages, not cleaning up those tests. If someone else like you or Junio
thinks that it would be a good time to clean things up a bit, then I
am ok with it, but that's not my main goal here.
> - the graph is definitely written in an unnecessarily different format
> than in the same test script!!!
Just above you complain about things that are similar to the previous
tests and now you complain about things that are different...
> - speaking of the graph: there is a perfectly fine commit graph already.
> Why on earth is it not reused?
Perhaps because it is more complex than needed to test this feature
and/or to understand what happens. And I don't think we require
everywhere only one commit graph per test script.
> In this particular case it even feels as if this test is not even testing
> what it should test at all:
>
> - it should verify that all of the commits in the first parent lineage are
> part of the list
It does that.
> - it should verify that none of the other commits are in the list
It does that too.
> And that is really all there is to test.
I don't agree. I think that when possible, especially when testing
plumbing commands like rev-list, it is a good thing to test as many
things as possible at once.
> You can even write that in a much
> easier way:
>
> -- snip --
> test_expect_success '--first-parent --bisect-all lists correct revs' '
> git rev-list --first-parent --bisect-all F..E >revs &&
> # E and e1..e8 need to be shown, F and f1..f4 not
> test_line_count = 9 revs &&
> for rev in E e1 e2 e3 e4 e5 e6 e7 e8
> do
> grep "^$(git rev-parse $rev) " revs || {
> echo "$rev not shown"
Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells
Hi Eric, On Tue, Jun 26, 2018, 2:31 AM Eric Sunshine wrote: > > On Tue, Jun 26, 2018 at 5:20 AM Elijah Newren wrote: > > On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine > > wrote: > > > Aside from identifying a rather significant number of &&-chain breaks, > > > repairing those broken chains uncovered genuine bugs in several tests > > > which were hidden by missing &&-chain links. Those bugs are also fixed > > > by this series. I would appreciate if the following people would > > > double-check my fixes: > > > > > > Stefan Bellar - 8/29 "t7400" and (especially) 13/29 "lib-submodule-update" > > > Jonathan Tan - 10/29 "t9001" > > > Elijah Newren - 6/29 "t6036" > > > > Commented on the patch in question; 6/29 looks good. > > > > I also looked over the rest of the series. Apart from the ones you > > specifically called out as needing review by others besides me, and > > the final patch which makes me feel like a sed neophyte, all but one > > patch looked good to me. I just have a small question for that > > remaining patch, which I posted there. > > I guess you refer to your question[1] about whether test_must_fail() > is the correct choice over test_expect_code(). I just responded[2] > with a hopefully satisfactory answer. Yes, it does. Thanks!
Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect
Hi Chris,
On Tue, 26 Jun 2018, Christian Couder wrote:
> On Mon, Jun 25, 2018 at 7:33 PM, Junio C Hamano wrote:
> > Tiago Botelho writes:
> >
> >> +test_expect_success "--bisect-all --first-parent" '
> >> +cat >expect1 < >> +$(git rev-parse CC) (dist=2)
> >> +$(git rev-parse EX) (dist=1)
> >> +$(git rev-parse D) (dist=1)
> >> +$(git rev-parse FX) (dist=0)
> >> +EOF
> >> +
> >> +cat >expect2 < >> +$(git rev-parse CC) (dist=2)
> >> +$(git rev-parse D) (dist=1)
> >> +$(git rev-parse EX) (dist=1)
> >> +$(git rev-parse FX) (dist=0)
> >> +EOF
> >> +
> >> +git rev-list --bisect-all --first-parent FX ^A >actual &&
> >> + ( test_cmp expect1 actual || test_cmp expect2 actual )
> >> +'
> >
> > I hate to say this, but the above looks like a typical
> > unmaintainable mess.
> >
> > What happens when you or somebody else later needs to update the
> > graph to be tested to add one more commit (or even more)? Would it
> > be enough to add another "rev-parse" plus "dist=X" line in both
> > expects? Or do we see a trap for combinatorial explosion that
> > requires us to add new expect$N?
>
> What about the following then:
>
> test_dist_order () {
> file="$1"
> n="$2"
> while read -r hash dist
> do
> d=$(echo "$dist" | sed -e "s/(dist=\(.*\))/\1/")
> case "$d" in
> ''|*[!0-9]*) return 1 ;;
> *) ;;
> esac
> test "$d" -le "$n" || return 1
> n="$d"
> done <"$file"
> }
>
> test_expect_success "--bisect-all --first-parent" '
> cat >expect < $(git rev-parse CC) (dist=2)
> $(git rev-parse EX) (dist=1)
> $(git rev-parse D) (dist=1)
> $(git rev-parse FX) (dist=0)
> EOF
> sort expect >expect_sorted &&
> git rev-list --bisect-all --first-parent FX ^A >actual &&
> sort actual >actual_sorted &&
> test_cmp expect_sorted actual_sorted &&
> test_dist_order actual 2
> '
>
> This feels overkill to me, but it should scale if we ever make more
> complex tests.
I *think* that you misunderstand Junio. At least when I read this:
> $(git rev-parse CC) (dist=2)
> $(git rev-parse EX) (dist=1)
> $(git rev-parse D) (dist=1)
> $(git rev-parse FX) (dist=0)
I go: Huh?!?!?!?!?!
What's CC? Is it Christian Couder? Creative Commons? Crudely Complex?
The point, for me, is: if this test fails, at some stage in the future,
for any reason, it will be a major pain to even dissect what the test is
supposed to do. This is no good. And you can do better. A lot better. You
can write the test in a way that the head of a reader does not hurt, and
at the same time it is still obvious what it does, and obvious that it
does the right thing.
One thing that makes the brain stumble for certain is when you deviate
from the conventions, especially when it is for no good reason at all. In
this case (and I am not sure why you, as a long-time contributor, did not
spot this before public review):
- the titles of the test cases leave a lot of room for improvement,
- the lines are too long,
- the convention to end the `test_expect_success` line with an opening
single quote is not heeded,
- the graph is definitely written in an unnecessarily different format
than in the same test script!!!
- speaking of the graph: there is a perfectly fine commit graph already.
Why on earth is it not reused?
In this particular case it even feels as if this test is not even testing
what it should test at all:
- it should verify that all of the commits in the first parent lineage are
part of the list
- it should verify that none of the other commits are in the list
And that is really all there is to test. You can even write that in a much
easier way:
-- snip --
test_expect_success '--first-parent --bisect-all lists correct revs' '
git rev-list --first-parent --bisect-all F..E >revs &&
# E and e1..e8 need to be shown, F and f1..f4 not
test_line_count = 9 revs &&
for rev in E e1 e2 e3 e4 e5 e6 e7 e8
do
grep "^$(git rev-parse $rev) " revs || {
echo "$rev not shown" >&2 &&
return 1
}
done
'
-- snap --
To test more precisely for the order or the distance would be both
overkill and likely to be unreadable.
To test `--bisect-vars` here would be excessive, as the part that handles
that particular option is not even touched. All that is touched is the
logic in the bisect algorithm in conjunction with --first-parent. And that
is all that should be tested here.
With a test like the one I outlined above, I only have one more gripe
about the patch: the commit message does nothing to explain this part of
the diff:
+ if ((bisect_flags & BISECT_FIRST_PARENT)) {
+ if (weight(q) < 0)
+ q = NULL;
+ break;
+ }
And I would really, really like that to be explained in the comm
git rerere and diff3
hi there, i have noticed that merge.conflictstyle has an impact on the rerere resolution. looking briefly at the source code, it seems that git tries to discard the common ancestor diff3 bits, but what I am seeing is that if i do the following then it fails: 1. from a clean rr-cache state, with merge.conflictsytle=diff3, git merge , resolve the conflicts, then commit 2. undo the previous merge, remove merge.conflictstyle=diff3 (disable diff3) and merge the *same* branch, then rerere won't fix the conflicts Is that the expected behavior? I am not familiar with git code, but browsing rerere.c makes me think that it should have worked.. Of course, if I merge the same branch without modifying merge.conflictstyle, then the merge conflicts are properly resolved by rerere. thanks!
Re: [GSoC][PATCH 1/1] sequencer: print an error message if append_todo_help() fails
Hi Alban, On Tue, 26 Jun 2018, Alban Gruin wrote: > This adds an error when append_todo_help() fails to write its message to > the todo file. > > Signed-off-by: Alban Gruin ACK. We *may* want to fold that into the commit that adds `append_todo_help()`. And, as I mentioned previously, I would love for that function to be used as an excuse to introduce the long-overdue `interactive-rebase.c` (`sequencer.c` is supposed to be the backend for cherry-pick and revert and interactive rebase, but not a catch-all for *all* of those things, it is already way too long to be readable, and I take blame for a large part of that.) Ciao, Dscho
Re: curious about wording in "man git-config", ENVIRONMENT
On Tue, Jun 26, 2018 at 06:18:26AM -0400, Robert P. J. Day wrote: > > ENVIRONMENT > GIT_CONFIG > Take the configuration from the given file instead of > .git/config. Using the "--global" option forces this to > ~/.gitconfig. Using the "--system" option forces this to > $(prefix)/etc/gitconfig. > > is the phrase "forces this to" really what you want to use here? > maybe i misunderstand what this option does, doesn't it simply mean > that it will use a different (specified) file from the default, > depending on the context (local, global, system)? > > it just seems weird to say that the option "forces" the use of what > are clearly the default files. thoughts? I agree it's weird. I think it's trying to mean "behaves as if it was set to", but with the additional notion that the command-line argument would take precedence over the environment (which is our usual rule). But then we should just say those things explicitly. Just looking at mentions of GIT_CONFIG in that manpage and knowing the history, I think: - the environment section should say something like: GIT_CONFIG If set and no other specific-file options are given, behaves as if `--file=$GIT_CONFIG` was provided on the command-line. - possibly the manpage should mention that GIT_CONFIG is historical and should not be used in new code (we could also consider an actual deprecation period and removal of the feature, though aside from documentation confusion I do not think it is hurting anyone) - the description of --file should not mention it at all. Instead it should reference the "FILES" section which describes the whole lookup sequence - mention of GIT_CONFIG should be dropped from the FILES section. We don't want to point people towards using it. And if they come across it in the wild, they can find it in the ENVIRONMENT section. - references to "--global" should stop mentioning ~/.gitconfig, since in a post-XDG world it could be elsewhere (they're better to refer to the "--global" description or the FILES section) - references to "--system" should stop mentioning $(prefix)/etc/gitconfig, since it can be configured separately from the prefix (and in most packaged builds which set prefix=/usr, $(sysconfdir) is not $(prefix)/etc). -Peff
