* Re: in 2.19.2 t0061-run-command FAILs if . is in $PATH
From: Johannes Schindelin @ 2018-11-29 10:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: H.Merijn Brand, git
In-Reply-To: <xmqqpnuor6qs.fsf@gitster-ct.c.googlers.com>
Hi Merijn and Junio,
On Thu, 29 Nov 2018, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > -test_expect_success 'run_command is restricted to PATH' '
> > +test_lazy_prereq DOT_IN_PATH '
> > + case ":$PATH:" in
> > + *:.:*) true;;
> > + *) false;;
> > + esac
> > +'
>
> An empty element in the colon-separated list also serves as an
> instruction to pick up executable from $cwd, so
>
> case ":$PATH:" in
> *:.:** | *::*) true ;;
> *) false ;;
> esac
>
> perhaps.
Good point.
Merijn, please be sure to squash this fix in before you submit the final
thing.
Thanks,
Johannes
>
> > +test_expect_success !DOT_IN_PATH 'run_command is restricted to PATH' '
> > write_script should-not-run <<-\EOF &&
> > echo yikes
> > EOF
> > -- snap --
> >
> > If so, can you please provide a commit message for it (you can add my
> > Signed-off-by: line and your Tested-by: line).
> >
> > Thanks,
> > Johannes
>
^ permalink raw reply
* Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
From: Johannes Schindelin @ 2018-11-29 10:07 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano, Eric Sunshine
In-Reply-To: <20181128201852.9782-3-avarab@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 9368 bytes --]
Hi Ævar,
On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> Change the semantics of the "--range-diff" option so that the regular
> diff options can be provided separately for the range-diff and the
> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
> "format-patch" to provide different context for the range-diff and the
> patch. This wasn't possible before.
I really, really dislike the `--range-diff-<random-thing>`. We have
precedent for passing optional arguments that are passed to some other
command, so a much more logical and consistent convention would be to use
`--range-diff[=<diff-option>..]`, allowing all of the diff options that
you might want to pass to the outer diff in one go rather than having a
lengthy string of `--range-diff-this` and `--range-diff-that` options.
I only had time to skim the patch, and I have to wonder why you pass
around full-blown `rev_info` structs for range diff (and with that really
awful name `rd_rev`) rather than just the `diff_options` that you
*actually* care about?
Ciao,
Dscho
>
> Ever since the "--range-diff" option was added in
> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
> machinery has been the one we get from "format-patch"'s own
> setup_revisions().
>
> This sort of thing is unique among the log-like commands in
> builtin/log.c, no command than format-patch will embed the output of
> another log-like command. Since the "rev->diffopt" is reused we need
> to munge it before we pass it to show_range_diff(). See
> 43dafc4172 ("format-patch: don't include --stat with --range-diff
> output", 2018-11-22) for a related regression fix which is being
> mostly reverted here.
>
> Implementation notes: 1) We're not bothering with the full teardown
> around die() and will leak memory, but it's too much boilerplate to do
> all the frees with/without the die() and not worth it. 2) We call
> repo_init_revisions() for "rd_rev" even though we could get away with
> a shallow copy like the code we're replacing (and which
> show_range_diff() itself does). This is to make this code more easily
> understood.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> Documentation/git-format-patch.txt | 10 ++++++-
> builtin/log.c | 42 +++++++++++++++++++++++-------
> t/t3206-range-diff.sh | 41 +++++++++++++++++++++++++++++
> 3 files changed, 82 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index aba4c5febe..6c048f415f 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -24,7 +24,8 @@ SYNOPSIS
> [--to=<email>] [--cc=<email>]
> [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
> [--interdiff=<previous>]
> - [--range-diff=<previous> [--creation-factor=<percent>]]
> + [--range-diff=<previous> [--creation-factor=<percent>]
> + [--range-diff<common diff option>]]
> [--progress]
> [<common diff options>]
> [ <since> | <revision range> ]
> @@ -257,6 +258,13 @@ feeding the result to `git send-email`.
> creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
> for details.
>
> +--range-diff<common diff option>::
> + Other options prefixed with `--range-diff` are stripped of
> + that prefix and passed as-is to the diff machinery used to
> + generate the range-diff, e.g. `--range-diff-U0` and
> + `--range-diff--no-color`. This allows for adjusting the format
> + of the range-diff independently from the patch itself.
> +
> --notes[=<ref>]::
> Append the notes (see linkgit:git-notes[1]) for the commit
> after the three-dash line.
> diff --git a/builtin/log.c b/builtin/log.c
> index 02d88fa233..7658e56ecc 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
> fprintf(rev->diffopt.file, "\n");
> }
>
> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
> + int use_stdout,
> struct commit *origin,
> int nr, struct commit **list,
> const char *branch_name,
> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
> }
>
> if (rev->rdiff1) {
> - struct diff_options opts;
> - memcpy(&opts, &rev->diffopt, sizeof(opts));
> - opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
> -
> fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
> show_range_diff(rev->rdiff1, rev->rdiff2,
> - rev->creation_factor, 1, &opts);
> + rev->creation_factor, 1, &rd_rev->diffopt);
> }
> }
>
> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> struct commit *commit;
> struct commit **list = NULL;
> struct rev_info rev;
> + struct rev_info rd_rev;
> struct setup_revision_opt s_r_opt;
> int nr = 0, total, i;
> int use_stdout = 0;
> @@ -1603,6 +1601,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> init_log_defaults();
> git_config(git_format_config, NULL);
> repo_init_revisions(the_repository, &rev, prefix);
> + repo_init_revisions(the_repository, &rd_rev, prefix);
> rev.commit_format = CMIT_FMT_EMAIL;
> rev.expand_tabs_in_log_default = 0;
> rev.verbose_header = 1;
> @@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> rev.preserve_subject = keep_subject;
>
> argc = setup_revisions(argc, argv, &rev, &s_r_opt);
> - if (argc > 1)
> - die(_("unrecognized argument: %s"), argv[1]);
> + if (argc > 1) {
> + struct argv_array args = ARGV_ARRAY_INIT;
> + const char *prefix = "--range-diff";
> + int have_prefix = 0;
> +
> + for (i = 0; i < argc; i++) {
> + struct strbuf sb = STRBUF_INIT;
> + char *str;
> +
> + strbuf_addstr(&sb, argv[i]);
> + if (starts_with(argv[i], prefix)) {
> + have_prefix = 1;
> + strbuf_remove(&sb, 0, strlen(prefix));
> + }
> + str = strbuf_detach(&sb, NULL);
> + strbuf_release(&sb);
> +
> + argv_array_push(&args, str);
> + }
> +
> + if (!have_prefix)
> + die(_("unrecognized argument: %s"), argv[1]);
> + argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
> + if (argc > 1)
> + die(_("unrecognized argument: %s"), argv[1]);
> + }
>
> if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
> die(_("--name-only does not make sense"));
> @@ -1702,7 +1725,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> if (!use_patch_format &&
> (!rev.diffopt.output_format ||
> rev.diffopt.output_format == DIFF_FORMAT_PATCH))
> - /* Needs to be mirrored in show_range_diff() invocation */
> rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
> if (!rev.diffopt.stat_width)
> rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
> @@ -1877,7 +1899,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> if (cover_letter) {
> if (thread)
> gen_message_id(&rev, "cover");
> - make_cover_letter(&rev, use_stdout,
> + make_cover_letter(&rev, &rd_rev, use_stdout,
> origin, nr, list, branch_name, quiet);
> print_bases(&bases, rev.diffopt.file);
> print_signature(rev.diffopt.file);
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index bc5facc1cd..6916103888 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -308,6 +308,35 @@ test_expect_success 'format-patch with <common diff option>' '
> --range-diff=topic~..topic changed~..changed >actual.raw &&
> sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
> sed -e "s|:$||" >expect <<-\EOF &&
> + 1: a63e992 ! 1: d966c5c s/12/B/
> + @@ -8,7 +8,7 @@
> + @@
> + 9
> + 10
> + - B
> + + BB
> + -12
> + +B
> + 13
> + -- :
> + EOF
> + test_cmp expect actual.range-diff &&
> + sed -ne "/^--- /,/^--/p" <actual.raw >actual.diff &&
> + sed -e "s|:$||" >expect <<-\EOF &&
> + --- a/file
> + +++ b/file
> + @@ -12 +12 @@ BB
> + -12
> + +B
> + -- :
> + EOF
> + test_cmp expect actual.diff &&
> +
> + # -U0 & --range-diff-U0
> + git format-patch --cover-letter --stdout -U0 --range-diff-U0 \
> + --range-diff=topic~..topic changed~..changed >actual.raw &&
> + sed -ne "/^1:/,/^--/p" <actual.raw >actual.range-diff &&
> + sed -e "s|:$||" >expect <<-\EOF &&
> 1: a63e992 ! 1: d966c5c s/12/B/
> @@ -11 +11 @@
> - B
> @@ -327,4 +356,16 @@ test_expect_success 'format-patch with <common diff option>' '
> test_cmp expect actual.diff
> '
>
> +test_expect_success 'format-patch option parsing with --range-diff-*' '
> + test_must_fail git format-patch --stdout --unknown \
> + master..unmodified 2>stderr &&
> + test_i18ngrep "unrecognized argument: --unknown" stderr &&
> + test_must_fail git format-patch --stdout --range-diff-unknown \
> + master..unmodified 2>stderr &&
> + test_i18ngrep "unrecognized argument: --range-diff-unknown" stderr &&
> + test_must_fail git format-patch --stdout --unknown --range-diff-unknown \
> + master..unmodified 2>stderr &&
> + test_i18ngrep "unrecognized argument: --unknown" stderr
> +'
> +
> test_done
> --
> 2.20.0.rc1.387.gf8505762e3
>
>
^ permalink raw reply
* Re: [PATCH] pack-protocol.txt: accept error packets in any context
From: Junio C Hamano @ 2018-11-29 8:42 UTC (permalink / raw)
To: Masaya Suzuki; +Cc: git
In-Reply-To: <20181127045301.103807-1-masayasuzuki@google.com>
Masaya Suzuki <masayasuzuki@google.com> writes:
> In the Git pack protocol definition, an error packet may appear only in
> a certain context. However, servers can face a runtime error (e.g. I/O
> error) at an arbitrary timing. This patch changes the protocol to allow
> an error packet to be sent instead of any packet.
>
> Following this protocol spec change, the error packet handling code is
> moved to pkt-line.c.
>
> Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
> ---
Have you run tests with this applied? I noticed 5703.9 now has
stale expectations, but there may be others.
^ permalink raw reply
* Re: [PATCH] rebase -i: introduce the 'test' command
From: Johannes Schindelin @ 2018-11-29 8:32 UTC (permalink / raw)
To: Paul Morelle; +Cc: Git Users
In-Reply-To: <25e07b91-3089-153c-2ecf-7d2d66bc3b65@gmail.com>
Hi Paul,
On Wed, 28 Nov 2018, Paul Morelle wrote:
> On 28/11/18 16:19, Johannes Schindelin wrote:
> >
> > On Wed, 28 Nov 2018, Paul Morelle wrote:
> >
> >> The 'exec' command can be used to run tests on a set of commits,
> >> interrupting on failing commits to let the user fix the tests.
> >>
> >> However, the 'exec' line has been consumed, so it won't be ran again by
> >> 'git rebase --continue' is ran, even if the tests weren't fixed.
> >>
> >> This commit introduces a new command 'test' equivalent to 'exec', except
> >> that it is automatically rescheduled in the todo list if it fails.
> >>
> >> Signed-off-by: Paul Morelle <paul.morelle@gmail.com>
> > Would it not make more sense to add a command-line option (and a config
> > setting) to re-schedule failed `exec` commands? Like so:
>
> Your proposition would do in most cases, however it is not possible to
> make a distinction between reschedulable and non-reschedulable commands.
True. But I don't think that's so terrible.
What I think is something to avoid is two commands that do something very,
very similar, but with two very, very different names.
In reality, I think that it would even make sense to change the default to
reschedule failed `exec` commands. Which is why I suggested to also add a
config option.
> Do you think that we can ignore the distinction between reschedulable
> and non-reschedulable commands in the same script?
Yes, I think that there *should not* be any distinction. It would just
make it harder to use the feature (users would have to keep in mind that
one version reschedules, one version does not).
> In this case, I would add some tests to your patch below, and propose
> the result on this mailing list.
I already added a test... See the reschedule-failed-exec branch on
https://github.com/dscho/git.
And I had another idea. To make this feature more useful for *myself*, I
would like to introduce the `-X` option as a shortcut for
`--reschedule-failed-exec -x`, e.g.
git rebase -X "make -j15 test" <base>
What do you think?
Johannes
>
> > -- snip --
> > diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
> > index a2ab68ed0632..a9ab009fdbca 100644
> > --- a/builtin/rebase--interactive.c
> > +++ b/builtin/rebase--interactive.c
> > @@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
> > OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
> > OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
> > OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
> > + OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
> > + N_("automatically re-schedule any `exec` that fails")),
> > OPT_END()
> > };
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 5b3e5baec8a0..700cbc4e150e 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -104,6 +104,7 @@ struct rebase_options {
> > int rebase_merges, rebase_cousins;
> > char *strategy, *strategy_opts;
> > struct strbuf git_format_patch_opt;
> > + int reschedule_failed_exec;
> > };
> >
> > static int is_interactive(struct rebase_options *opts)
> > @@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options *opts)
> > argv_array_push(&child.args, opts->gpg_sign_opt);
> > if (opts->signoff)
> > argv_array_push(&child.args, "--signoff");
> > + if (opts->reschedule_failed_exec)
> > + argv_array_push(&child.args, "--reschedule-failed-exec");
> >
> > status = run_command(&child);
> > goto finished_rebase;
> > @@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> > "strategy")),
> > OPT_BOOL(0, "root", &options.root,
> > N_("rebase all reachable commits up to the root(s)")),
> > + OPT_BOOL(0, "reschedule-failed-exec",
> > + &options.reschedule_failed_exec,
> > + N_("automatically re-schedule any `exec` that fails")),
> > OPT_END(),
> > };
> > int i;
> > @@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> > break;
> > }
> >
> > + if (options.reschedule_failed_exec && !is_interactive(&options))
> > + die(_("--reschedule-failed-exec requires an interactive rebase"));
> > +
> > if (options.git_am_opts.argc) {
> > /* all am options except -q are compatible only with --am */
> > for (i = options.git_am_opts.argc - 1; i >= 0; i--)
> > diff --git a/sequencer.c b/sequencer.c
> > index e1a4dd15f1a8..69bee63e116f 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
> > static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
> > static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
> > static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
> > +static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
> >
> > static int git_sequencer_config(const char *k, const char *v, void *cb)
> > {
> > @@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts)
> > opts->signoff = 1;
> > }
> >
> > + if (file_exists(rebase_path_reschedule_failed_exec()))
> > + opts->reschedule_failed_exec = 1;
> > +
> > read_strategy_opts(opts, &buf);
> > strbuf_release(&buf);
> >
> > @@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
> > write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);
> > if (opts->signoff)
> > write_file(rebase_path_signoff(), "--signoff\n");
> > + if (opts->reschedule_failed_exec)
> > + write_file(rebase_path_reschedule_failed_exec(), "%s", "");
> >
> > return 0;
> > }
> > @@ -3586,9 +3592,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
> > *end_of_arg = saved;
> >
> > /* Reread the todo file if it has changed. */
> > - if (res)
> > - ; /* fall through */
> > - else if (stat(get_todo_path(opts), &st))
> > + if (res) {
> > + if (opts->reschedule_failed_exec)
> > + reschedule = 1;
> > + } else if (stat(get_todo_path(opts), &st))
> > res = error_errno(_("could not stat '%s'"),
> > get_todo_path(opts));
> > else if (match_stat_data(&todo_list->stat, &st)) {
> > diff --git a/sequencer.h b/sequencer.h
> > index 5071a73563f1..1f865dae2618 100644
> > --- a/sequencer.h
> > +++ b/sequencer.h
> > @@ -39,6 +39,7 @@ struct replay_opts {
> > int allow_empty_message;
> > int keep_redundant_commits;
> > int verbose;
> > + int reschedule_failed_exec;
> >
> > int mainline;
> >
> > -- snap --
> >
> > That would avoid your having to add a `--test` option accompanying the `--exec`
> > option (which is missing from your patch).
>
> You are right about the missing `--test` option, which I could also add
> to a new version of this patch.
>
> Cheers,
> Paul
>
> > Ciao,
> > Johannes
> >
> >> ---
> >> Documentation/git-rebase.txt | 9 ++++++---
> >> rebase-interactive.c | 1 +
> >> sequencer.c | 23 +++++++++++++++--------
> >> t/lib-rebase.sh | 4 +++-
> >> t/t3404-rebase-interactive.sh | 16 ++++++++++++++++
> >> 5 files changed, 41 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> >> index 80793bad8..c8f565637 100644
> >> --- a/Documentation/git-rebase.txt
> >> +++ b/Documentation/git-rebase.txt
> >> @@ -693,8 +693,8 @@ $ git rebase -i -p --onto Q O
> >> Reordering and editing commits usually creates untested intermediate
> >> steps. You may want to check that your history editing did not break
> >> anything by running a test, or at least recompiling at intermediate
> >> -points in history by using the "exec" command (shortcut "x"). You may
> >> -do so by creating a todo list like this one:
> >> +points in history by using the "exec" command (shortcut "x") or the
> >> +"test" command. You may do so by creating a todo list like this one:
> >> -------------------------------------------
> >> pick deadbee Implement feature XXX
> >> @@ -702,7 +702,7 @@ fixup f1a5c00 Fix to feature XXX
> >> exec make
> >> pick c0ffeee The oneline of the next commit
> >> edit deadbab The oneline of the commit after
> >> -exec cd subdir; make test
> >> +test cd subdir; make test
> >> ...
> >> -------------------------------------------
> >> @@ -715,6 +715,9 @@ in `$SHELL`, or the default shell if `$SHELL` is
> >> not set), so you can
> >> use shell features (like "cd", ">", ";" ...). The command is run from
> >> the root of the working tree.
> >> +The "test" command does the same, but will remain in the todo list as
> >> +the next command, until it succeeds.
> >> +
> >> ----------------------------------
> >> $ git rebase -i --exec "make test"
> >> ----------------------------------
> >> diff --git a/rebase-interactive.c b/rebase-interactive.c
> >> index 78f3263fc..4a408661d 100644
> >> --- a/rebase-interactive.c
> >> +++ b/rebase-interactive.c
> >> @@ -14,6 +14,7 @@ void append_todo_help(unsigned edit_todo, unsigned
> >> keep_empty,
> >> "s, squash <commit> = use commit, but meld into previous commit\n"
> >> "f, fixup <commit> = like \"squash\", but discard this commit's log
> >> message\n"
> >> "x, exec <command> = run command (the rest of the line) using shell\n"
> >> +" test <command> = same as exec command, but keep it in TODO if it
> >> fails\n"
> >> "b, break = stop here (continue rebase later with 'git rebase
> >> --continue')\n"
> >> "d, drop <commit> = remove commit\n"
> >> "l, label <label> = label current HEAD with a name\n"
> >> diff --git a/sequencer.c b/sequencer.c
> >> index e1a4dd15f..c3dde6910 100644
> >> --- a/sequencer.c
> >> +++ b/sequencer.c
> >> @@ -1508,6 +1508,7 @@ enum todo_command {
> >> TODO_SQUASH,
> >> /* commands that do something else than handling a single commit */
> >> TODO_EXEC,
> >> + TODO_TEST,
> >> TODO_BREAK,
> >> TODO_LABEL,
> >> TODO_RESET,
> >> @@ -1530,6 +1531,7 @@ static struct {
> >> { 'f', "fixup" },
> >> { 's', "squash" },
> >> { 'x', "exec" },
> >> + { 0, "test" },
> >> { 'b', "break" },
> >> { 'l', "label" },
> >> { 't', "reset" },
> >> @@ -2072,7 +2074,7 @@ static int parse_insn_line(struct todo_item *item,
> >> const char *bol, char *eol)
> >> command_to_string(item->command));
> >> if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
> >> - item->command == TODO_RESET) {
> >> + item->command == TODO_RESET || item->command == TODO_TEST) {
> >> item->commit = NULL;
> >> item->arg = bol;
> >> item->arg_len = (int)(eol - bol);
> >> @@ -3576,7 +3578,7 @@ static int pick_commits(struct todo_list
> >> *todo_list, struct replay_opts *opts)
> >> item->arg, item->arg_len, opts,
> >> res, to_amend);
> >> }
> >> - } else if (item->command == TODO_EXEC) {
> >> + } else if (item->command == TODO_EXEC || item->command == TODO_TEST) {
> >> char *end_of_arg = (char *)(item->arg + item->arg_len);
> >> int saved = *end_of_arg;
> >> struct stat st;
> >> @@ -3586,9 +3588,12 @@ static int pick_commits(struct todo_list
> >> *todo_list, struct replay_opts *opts)
> >> *end_of_arg = saved;
> >> /* Reread the todo file if it has changed. */
> >> - if (res)
> >> + if (res) {
> >> ; /* fall through */
> >> - else if (stat(get_todo_path(opts), &st))
> >> + if (item->command == TODO_TEST) {
> >> + reschedule = 1;
> >> + }
> >> + } else if (stat(get_todo_path(opts), &st))
> >> res = error_errno(_("could not stat '%s'"),
> >> get_todo_path(opts));
> >> else if (match_stat_data(&todo_list->stat, &st)) {
> >> @@ -3622,10 +3627,12 @@ static int pick_commits(struct todo_list
> >> *todo_list, struct replay_opts *opts)
> >> return error(_("unknown command %d"), item->command);
> >> if (reschedule) {
> >> - advise(_(rescheduled_advice),
> >> - get_item_line_length(todo_list,
> >> - todo_list->current),
> >> - get_item_line(todo_list, todo_list->current));
> >> + if (item->command != TODO_TEST) {
> >> + advise(_(rescheduled_advice),
> >> + get_item_line_length(todo_list,
> >> + todo_list->current),
> >> + get_item_line(todo_list, todo_list->current));
> >> + }
> >> todo_list->current--;
> >> if (save_todo(todo_list, opts))
> >> return -1;
> >> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> >> index 7ea30e500..7d36f00bd 100644
> >> --- a/t/lib-rebase.sh
> >> +++ b/t/lib-rebase.sh
> >> @@ -19,6 +19,8 @@
> >> #
> >> # "exec_cmd_with_args" -- add an "exec cmd with args" line.
> >> #
> >> +# "test_cmd_with_args" -- add a "test cmd with args" line.
> >> +#
> >> # "#" -- Add a comment line.
> >> #
> >> # ">" -- Add a blank line.
> >> @@ -49,7 +51,7 @@ set_fake_editor () {
> >> case $line in
> >> pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
> >> action="$line";;
> >> - exec_*|x_*|break|b)
> >> + exec_*|x_*|break|b|test_*)
> >> echo "$line" | sed 's/_/ /g' >> "$1";;
> >> "#")
> >> echo '# comment' >> "$1";;
> >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> >> index 7a440e08d..14c60b14d 100755
> >> --- a/t/t3404-rebase-interactive.sh
> >> +++ b/t/t3404-rebase-interactive.sh
> >> @@ -1453,4 +1453,20 @@ test_expect_success 'valid author header when
> >> author contains single quote' '
> >> test_cmp expected actual
> >> '
> >> +test_expect_success 'rebase -i keeps test until it passes' '
> >> + git checkout master &&
> >> + (
> >> + set_fake_editor &&
> >> + FAKE_LINES="1 test_false 2 3 4 5" &&
> >> + export FAKE_LINES &&
> >> + test_must_fail git rebase -i A &&
> >> + test_cmp_rev B HEAD &&
> >> + test_must_fail git rebase --continue &&
> >> + test_cmp_rev B HEAD &&
> >> + FAKE_LINES="test_true 2 3 4" git rebase --edit-todo &&
> >> + git rebase --continue
> >> + ) &&
> >> + test_cmp_rev master HEAD
> >> +'
> >> +
> >> test_done
> >> --
> >> 2.19.1
> >>
> >>
>
^ permalink raw reply
* Re: [PATCH] pack-protocol.txt: accept error packets in any context
From: Junio C Hamano @ 2018-11-29 7:49 UTC (permalink / raw)
To: Masaya Suzuki; +Cc: git
In-Reply-To: <xmqqh8g3t7gm.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
>> diff --git a/connect.c b/connect.c
>> index 24281b608..458906e60 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -306,8 +306,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,
>> die_initial_contact(1);
>> case PACKET_READ_NORMAL:
>> len = reader->pktlen;
>> - if (len > 4 && skip_prefix(reader->line, "ERR ", &arg))
>> - die(_("remote error: %s"), arg);
>> break;
>> case PACKET_READ_FLUSH:
>> state = EXPECTING_DONE;
This breaks build by not removing the decl of arg while removing the
last user of that variable in the function.
connect.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/connect.c b/connect.c
index 458906e60d..4813f005ab 100644
--- a/connect.c
+++ b/connect.c
@@ -296,7 +296,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,
struct ref **orig_list = list;
int len = 0;
enum get_remote_heads_state state = EXPECTING_FIRST_REF;
- const char *arg;
*list = NULL;
--
2.20.0-rc1-10-g7068cbc4ab
^ permalink raw reply related
* Re: [PATCH v2] log -G: Ignore binary files
From: Junio C Hamano @ 2018-11-29 7:22 UTC (permalink / raw)
To: Thomas Braun; +Cc: git, peff, sbeller, avarab
In-Reply-To: <xmqqa7lsnyu5.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
>> +test_expect_success 'log -G ignores binary files' '
>> + git checkout --orphan orphan1 &&
>> + printf "a\0a" >data.bin &&
>> + git add data.bin &&
>> + git commit -m "message" &&
>> + git log -Ga >result &&
>> + test_must_be_empty result
>> +'
>
> As this is the first mention of data.bin, this is adding a new file
> data.bin that has two 'a' but is a binary file. And that is the
> only commit in the history leading to orphan1.
>
> The fact that "log -Ga" won't find any means it missed the creation
> event, because the blob is binary. Good.
By the way, this root commit records another file whose path is
"file" and has "Picked<LF>" in it. If the file had 'a' in it, it
would have been included in "git log" output, but that is too subtle
a point to be noticed by the readers who are only reading this patch
without seeing what has been done to the index before this test
piece.
If you are going to restructure these tests to create a three-commit
history in a single expect_success that is inspected with various
"log -Ga" invocations in subsequent tests, it is worth removing that
other file (or rather, starting with "read-tree --empty" immediately
after checking out the orphan branch, to clarify to the readers that
there is nothing but what you add in the set-up step in the index)
to make the test more robust.
^ permalink raw reply
* Re: [PATCH v2] log -G: Ignore binary files
From: Junio C Hamano @ 2018-11-29 7:10 UTC (permalink / raw)
To: Thomas Braun; +Cc: git, peff, sbeller, avarab
In-Reply-To: <c4eac0b0ff0812e5aa8b081e603fc8bdd042ddeb.1543403143.git.thomas.braun@virtuell-zuhause.de>
Thomas Braun <thomas.braun@virtuell-zuhause.de> writes:
> Subject: Re: [PATCH v2] log -G: Ignore binary files
s/Ig/ig/; (will locally munge--this alone is no reason to reroll).
The code changes looked sensible.
> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
> index 844df760f7..5c3e2a16b2 100755
> --- a/t/t4209-log-pickaxe.sh
> +++ b/t/t4209-log-pickaxe.sh
> @@ -106,4 +106,44 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '
> rm .gitattributes
> '
>
> +test_expect_success 'log -G ignores binary files' '
> + git checkout --orphan orphan1 &&
> + printf "a\0a" >data.bin &&
> + git add data.bin &&
> + git commit -m "message" &&
> + git log -Ga >result &&
> + test_must_be_empty result
> +'
As this is the first mention of data.bin, this is adding a new file
data.bin that has two 'a' but is a binary file. And that is the
only commit in the history leading to orphan1.
The fact that "log -Ga" won't find any means it missed the creation
event, because the blob is binary. Good.
> +test_expect_success 'log -G looks into binary files with -a' '
> + git checkout --orphan orphan2 &&
> + printf "a\0a" >data.bin &&
> + git add data.bin &&
> + git commit -m "message" &&
This starts from the state left by the previous test piece, i.e. we
have a binary data.bin file with two 'a' in it. We pretend to
modify and add, but these two steps are no-op if the previous
succeeded, but even if the previous step failed, we get what we want
in the data.bin file. And then we make an initial commit the same
way.
> + git log -a -Ga >actual &&
> + git log >expected &&
And we ran the same test but this time with "-a" to tell Git that
binary-ness should not matter. It will find the sole commit. Good.
> + test_cmp actual expected
> +'
> +
> +test_expect_success 'log -G looks into binary files with textconv filter' '
> + git checkout --orphan orphan3 &&
> + echo "* diff=bin" > .gitattributes &&
s/> />/; (will locally munge--this alone is no reason to reroll).
> + printf "a\0a" >data.bin &&
> + git add data.bin &&
> + git commit -m "message" &&
> + git -c diff.bin.textconv=cat log -Ga >actual &&
This exposes a slight iffy-ness in the design. The textconv filter
used here does not strip the "binary-ness" from the payload, but it
is enough to tell the machinery that -G should look into the
difference. Is that really desirable, though?
IOW, if this weren't the initial commit (which is handled by the
codepath to special-case creation and deletion in diff_grep()
function), would "log -Ga" show it without "-a"? Should it?
I think this test piece (and probably the previous ones for "-a" vs
"no -a" without textconv, as well) should be using a history with
three commits, where
- the root commit introduces "a\0a" to data.bin (creation event)
- the second commit adds another instance of "a\0a" to data.bin
(forces comparison)
- the third commit removes data.bin (deletion event)
and make sure that the three are treated identically. If "log -Ga"
finds one (with the combination of other conditions like use of
textconv or -a option), it should find all three, and vice versa.
> + git log >expected &&
> + test_cmp actual expected
> +'
> +
> +test_expect_success 'log -S looks into binary files' '
> + git checkout --orphan orphan4 &&
> + printf "a\0a" >data.bin &&
> + git add data.bin &&
> + git commit -m "message" &&
> + git log -Sa >actual &&
> + git log >expected &&
> + test_cmp actual expected
> +'
Likewise. This would also benefit from a three-commit history.
Perhaps you can create such a history at the beginning of these
additions as another "setup -G/-S binary test" step and test
different variations in subsequent tests without the setup?
> test_done
^ permalink raw reply
* Re: [PATCH 5/5] test-lib: add support for GIT_TODO_TESTS
From: Junio C Hamano @ 2018-11-29 6:42 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git
In-Reply-To: <20181127225445.30045-5-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> -To skip tests, set the GIT_SKIP_TESTS variable. Individual tests can
> -be skipped:
> +To skip tests, set either the GIT_SKIP_TESTS or GIT_TODO_TESTS
> +variables. The difference is that with SKIP the tests won't be run at
> +all, whereas they will be run with TODO, but in success or failure
> +annotated as a TODO test.
It is entirely unclear what "They will be run with TODO" means.
I know what you want to achieve from the code change; instead of
just skipping, you want to cause as much side effect the skipped
test piece wants to make until it fails and dies, without taking
the remainder of the test down. And I kind of agree that sometimes
such a mode would be very useful (i.e. when you do not want to
bother separating such a failing test properly into the setup part
that would affect later tests and the one-thing-it-wants-to-test
part that can now be safely skipped). From the cursory look, the
code change in this patch is sensible realization of that idea.
Having said all that, I won't picking this up until next month ;-) I
really want to see that everybody is concentrating on making sure
that 2.20 is solid before playing with shiny new toys.
Thanks.
^ permalink raw reply
* Re: [PATCH] transport-helper.c: do not translate a string twice
From: Junio C Hamano @ 2018-11-29 6:30 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20181126195756.15537-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> My bad.
>
> transport-helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 7213fa0d32..bf225c698f 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -573,7 +573,7 @@ static int run_connect(struct transport *transport, struct strbuf *cmdbuf)
> fprintf(stderr, "Debug: Falling back to dumb "
> "transport.\n");
> } else {
> - die(_(_("unknown response to connect: %s")),
> + die(_("unknown response to connect: %s"),
> cmdbuf->buf);
Thanks.
^ permalink raw reply
* Re: [PATCH v2 5/7] checkout: split options[] array in three pieces
From: Junio C Hamano @ 2018-11-29 6:29 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: avarab, git, Stefan Beller, t.gummerer
In-Reply-To: <20181127165211.24763-6-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> +static struct option *add_switch_branch_options(struct checkout_opts *opts,
> + struct option *prevopts)
> +{
> + struct option options[] = {
> OPT_STRING('b', NULL, &opts->new_branch, N_("branch"),
> N_("create and checkout a new branch")),
I think there should be another step to rename the options to more
sensible ones for the context. In the context of overall "checkout"
command, the 'b' option
git checkout -b <new-name> <commit-ish>"
that signals that its parameter has something to do with a 'branch'
makes perfect sense. But when the user uses the new command
git switch-branch -b <new-name> <commit-ish>
does not convey the more important fact in the context. In the
orignal context, "this is about a branch" and "we are not checking
out an existing one, but are creating" are both worthwhile thing to
express, but because a single letter option cannot stand for both,
"-b" is the most reasonable choice (compared to calling it "-c" that
stands for "create" that invites "what exactly are you creating?").
In the new context of "switch-branch", it no longer has to be said
that the optional feature is about "branch". So I would imagine
that users naturally expect this option to be called
git switch-branch --create <new-name> <commit-ish>
(or -c for short).
I'll just stop with this single example, but I think we should make
sure all the options make sense in the context of new command.
Of course, that means it will NOT be sufficient to just split the
option table into two tables and stitch them together for the single
command. This option must stay to be "-b" (giving it a synonym
"--create-branch" is OK) in the context of "checkout".
^ permalink raw reply
* Re: [PATCH/RFC v2 0/7] Introduce new commands switch-branch and checkout-files
From: Junio C Hamano @ 2018-11-29 6:14 UTC (permalink / raw)
To: Stefan Xenos
Cc: pclouds, Ævar Arnfjörð Bjarmason, git,
Stefan Beller, t.gummerer
In-Reply-To: <CAPL8ZivFJXQw=yLXjOPxsjabwN5XAP_qe0LK3sODO4NkgCjZag@mail.gmail.com>
Stefan Xenos <sxenos@google.com> writes:
> So - IMO - detaching should always be an explicit action. Some options
> that occur to me:
>
> git switch-branch --detach
That is the most obvious way to spell it, and it is why we have "git
checkout --detach". If we were to split one half of "checkout" into
"switch-branch", I would support the idea to make switch-branch only
take a branch name and nothing else, allow it to take any commit-ish
to detach the HEAD at that commit in the history graph with the
--detach option". I also do not think anybody minds explaining the
resulting state to be "on an unnamed branch" (or is it "the" unnamed
branch? Given that HEAD@{} reflog is a singleton, perhaps the right
mental model is that there is only one unnamed branch per worktree).
> git detach
The detached HEAD state is not all that special to deserve a
separate command. After all, all history growing commands like
"commit", "cherry-pick", "rebase", "merge", etc. work the same way
on the unnamed branch.
> git switch-branch HEAD
I do not think this one is acceptable. "git checkout HEAD" does not
detach but works as if you said "git checkout master" (when on the
'master' branch). And you do not want "git switch-branch HEAD^0" to
be that explicit way to tell Git to detach the HEAD as that will
take us back to square one ("git checkout HEAD^0" is the more
concise and time-honored way to say "git checkout --detach HEAD").
^ permalink raw reply
* Re: [PATCH v2 7/7] Suggest other commands instead of "git checkout"
From: Junio C Hamano @ 2018-11-29 6:05 UTC (permalink / raw)
To: Duy Nguyen
Cc: Ævar Arnfjörð Bjarmason, Git Mailing List,
Stefan Beller, Thomas Gummerer
In-Reply-To: <CACsJy8BBeRDY85rJLzeJRFSazEn1rZ8JGN6_vwAr2ia=FaiLNg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> I see my deliberate attempt to provoke has failed :D Giving your view
> of the new commands as "training wheels", I take it we still should
> make them visible as much as possible, but we just not try to hide
> "git checkout" as much (e.g. we mention both new and old commands,
> instead of just mentioning the new one, when suggesting something)?
Yes, I do support the overall idea of learning two (or possibly
three) separate commands would help new users to form the right
mental model much sooner than learning one that can be used in
multiple ways. Another possible approach could be to split the use
of "reset" that does not move "HEAD" into the same half of
"checkout" that does not move "HEAD", i.e. checkout-files.
^ permalink raw reply
* Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files
From: Junio C Hamano @ 2018-11-29 5:59 UTC (permalink / raw)
To: Stefan Xenos
Cc: pclouds, Ævar Arnfjörð Bjarmason, git,
Stefan Beller, t.gummerer
In-Reply-To: <CAPL8ZivJ+=Y=8pxvs3sJrdxVtkn9xfTA63GeHcr=J0Y2JscOMQ@mail.gmail.com>
Stefan Xenos <sxenos@google.com> writes:
> Although I have no problem with "switch-branch" as a command name,
> some alternative names we might consider for switch-branch might be:
>
> chbranch
> swbranch
Please never go in that direction. So far, we made a conscious
effort to keep the names of most frequently used subcommand to
proper words that can be understood by coders (IOW, I expect they
know what 'grep' is, even though that may not be a 'proper word').
> switch
> branch change (as a subcommand for the "branch" command)
It is more like moving to the branch to work on. I think 'switch'
is something SVN veterans may find familiar. Both are much better
than ch/swbranch that are overly cryptic.
^ permalink raw reply
* Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files
From: Junio C Hamano @ 2018-11-29 5:55 UTC (permalink / raw)
To: Stefan Beller
Cc: Duy Nguyen, Ævar Arnfjörð Bjarmason, git,
Thomas Gummerer
In-Reply-To: <CAGZ79kbWqfMHZeYFXNh00N5xSSkW0_Mzja1EtuzxQxrhESoZxQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> I dislike the checkout-* names, as we already have checkout-index
> as plumbing, so it would be confusing as to which checkout-* command
> should be used when and why as it seems the co-index moves
> content *from index* to the working tree, but the co-files moves content
> *to files*, whereas checkout-branch is neither 'moving' to or from a branch
> but rather 'switching' to that branch.
To me, "switching to work on the branch", is like "checking the book
out from the library to read". IOW, "check the branch out to work
on" does not have to involve *any* moving of contents.
^ permalink raw reply
* Re: [PATCH 1/1] rebase --stat: fix when rebasing to an unrelated history
From: Junio C Hamano @ 2018-11-29 5:51 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <680385e4bd5c34a5fcf9651a776c52db61557652.1543317195.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> The built-in version of the `git rebase` command blindly translated that
> shell script code, assuming that there is no need to test whether there
> *was* a merge base, and due to its better error checking, exited with a
> fatal error (because it tried to read the object with hash 00000000...
> as a tree).
And the scripted version produced a wrong result because it did not
check the lack of merge base and fed an empty string (presumably, as
that is what you would get from mb=$(merge-base A B) when A and B
are unrelated) to later steps? If that is the case, then it *is* a
very significant thing to record here. As it was not merely "failed
to stop due to lack of error checking", but a lot worse. It was
producing a wrong result.
A more faithful reimplementation would not have exited with fatal
error, but would have produced the same wrong result, so "a better
error checking caused the reimplementation die where the original
did not die" may be correct but is much less important than the fact
that "the logic used in the original to produce diffstat forgot that
there may not be a merge base and would not have worked correctly in
such a case, and that is what we are correcting" is more important.
Please descibe the issue as such, if that is the case (I cannot read
from the description in the proposed log message if that is the
case---but I came to the conclusion that it is what you wanted to
fix reading the code).
> if (options.flags & REBASE_VERBOSE)
> printf(_("Changes from %s to %s:\n"),
> - oid_to_hex(&merge_base),
> + is_null_oid(&merge_base) ?
> + "(empty)" : oid_to_hex(&merge_base),
I am not sure (empty) is a good idea for two reasons. It is going
to be inserted into an translated string without translation. Also
it is too similar to the fallback string used by some printf
implementations when NULL was given to %s by mistake.
If there weren't i18n issues, I would suggest to use "empty merge
base" or "empty tree" instead, but the old one would have shown
0{40}, so perhaps empty_tree_oid_hex() is a good substitute?
> @@ -1494,8 +1495,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> opts.detect_rename = DIFF_DETECT_RENAME;
> diff_setup_done(&opts);
> - diff_tree_oid(&merge_base, &options.onto->object.oid,
> - "", &opts);
> + diff_tree_oid(is_null_oid(&merge_base) ?
> + the_hash_algo->empty_tree : &merge_base,
> + &options.onto->object.oid, "", &opts);
The original would have run "git diff '' $onto", and died with an
error message, so both the original and the reimplementation were
failing. Just in different ways ;-)
> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> index b97ffdc9dd..be3b241676 100755
> --- a/git-legacy-rebase.sh
> +++ b/git-legacy-rebase.sh
> @@ -718,10 +718,12 @@ if test -n "$diffstat"
> then
> if test -n "$verbose"
> then
> - echo "$(eval_gettext "Changes from \$mb to \$onto:")"
> + mb_display="${mb:-(empty)}"
> + echo "$(eval_gettext "Changes from \$mb_display to \$onto:")"
> fi
> + mb_tree="${mb:-$(git hash-object -t tree /dev/null)}"
> # We want color (if set), but no pager
> - GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
> + GIT_PAGER='' git diff --stat --summary "$mb_tree" "$onto"
Code fix for diff-tree invocation, both in the builtin/ version and
this one, looks correct.
^ permalink raw reply
* Re: [PATCH] config.mak.dev: enable -Wpedantic in clang
From: Junio C Hamano @ 2018-11-29 5:13 UTC (permalink / raw)
To: Eric Sunshine
Cc: Carlo Arenas, Git List, Beat Bolli,
Nguyễn Thái Ngọc Duy
In-Reply-To: <CAPig+cQXkNo_Sztf3y5ciZ3jjj=XrJ+ESCxEsU_RKxb_0x+STw@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> Playing Devi's Advocate, what if Apple's clang "8" was, in reality,
> real-world clang 3? Then this condition would incorrectly enable the
> compiler option on Apple for a (real) clang version below 4. For this
> reason, it seems we shouldn't be trusting only the clang version
> number when dealing with Apple.
>
> (I suspect that it won't make a difference in actual practice, so it
> may be reasonable to punt on this issue until/if someone complains.)
Why do we care which true version of clang is being used here in the
first place? Is it because some version of clang (take -Wpedantic
but misbehave | barf when -Wpedantic is given) while some others are
OK?
If the only problem is that some version of clang barf when the
option is given, perhaps we can do a trial-compile of helloworld.c
or something, or is that something we are trying to avoid in the
first place?
It appears to me that ./detect-compiler tool (by the way, perhaps we
should start moving these build-helper-tools sitting at the top level
down to ./build-helpers/ subdirectory or something so that we can focus
on the source code when running "ls" at the top level of the hierarchy)
can become more intelligent to help things like this.
^ permalink raw reply
* Re: [RFC PATCH] Introduce "precious" file concept
From: Junio C Hamano @ 2018-11-29 5:04 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Per Lundberg, brian m. carlson, git@vger.kernel.org, Steffen Jost,
Joshua Jensen, Matthieu Moy, Clemens Buchacher, Holger Hellmuth,
Kevin Ballard, Nguyễn Thái Ngọc Duy
In-Reply-To: <875zwgzx4v.fsf@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> I don't think something like the endgame you've described in
> https://public-inbox.org/git/xmqqzhtwuhpc.fsf@gitster-ct.c.googlers.com/
> is ever going to work. Novice git users (the vast majority) are not
> going to diligently update both .gitignore and some .gitattribute
> mechanism in lockstep.
That goes both ways, no? Forcing people to add the same pattern,
e.g. *.o, to .gitignore and .gitattribute to say they are
expendable, when most of the time they are, is not something you
should expect from the normal population.
>> I would think that a proper automation needs per-path hint from the
>> user and/or the project, not just a single-size-fits-all --force
>> option, and "unlike all the *.o ignored files that are expendable,
>> this vendor-supplied-object.o is not" is one way to give such a
>> per-path hint.
>>
>>> This would give scripts which relied on our stable plumbing consistent
>>> behavior, while helping users who're using our main porcelain not to
>>> lose data. I could then add a --force option to the likes of read-tree
>>> (on by default), so you could get porcelain-like behavior with
>>> --no-force.
>>
>> At that low level, I suspect that a single size fits all "--force"
>> would work even less well.
>
> Yeah I don't think the one-size-fits-all way out of this is a single
> --force flag.
Yes, indeed. That's why I prefer the "precious" bit. The system
would behave the same way with or without it, but projects (not
individual endusers) can take advantage of the feature if they
wanted to.
^ permalink raw reply
* Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
From: Junio C Hamano @ 2018-11-29 4:58 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Johannes Schindelin, Jonathan Nieder, git, Ian Jackson
In-Reply-To: <87bm69z939.fsf@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Since I raised this 'should we hold off?' I thought I'd chime in and say
> that I'm fine with going along with what you suggest and having the
> builtin as the default in the final. IOW not merge
> jc/postpone-rebase-in-c down.
OK.
^ permalink raw reply
* Re: [PATCH 0/5] Add a new "sparse" tree walk algorithm
From: Derrick Stolee @ 2018-11-29 4:05 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason,
Derrick Stolee via GitGitGadget
Cc: git, peff, jrnieder, Junio C Hamano
In-Reply-To: <874lc0zw0p.fsf@evledraar.gmail.com>
On 11/28/2018 5:18 PM, Ævar Arnfjörð Bjarmason wrote:
> This is really interesting. I tested this with:
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 124b1bafc4..5c7615f06c 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3143 +3143 @@ static void get_object_list(int ac, const char **av)
> - mark_edges_uninteresting(&revs, show_edge, sparse);
> + mark_edges_uninteresting(&revs, show_edge, 1);
>
> To emulate having a GIT_TEST_* mode for this, which seems like a good
> idea since it turned up a lot of segfaults in pack-objects. I wasn't
> able to get a backtrace for that since it always happens indirectly, and
> I didn't dig enough to see how to manually invoke it the right way.
Thanks for double-checking this. I had run a similar test in my
prototype implementation, but over-simplified some code when rewriting
it for submission (and then forgot to re-run that test).
Specifically, these null checks are important:
diff --git a/list-objects.c b/list-objects.c
index 9bb93d1640..7e864b4db8 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -232,6 +232,10 @@ static void add_edge_parents(struct commit *commit,
for (parents = commit->parents; parents; parents = parents->next) {
struct commit *parent = parents->item;
struct tree *tree = get_commit_tree(parent);
+
+ if (!tree)
+ continue;
+
oidset_insert(set, &tree->object.oid);
if (!(parent->object.flags & UNINTERESTING))
@@ -261,6 +265,8 @@ void mark_edges_uninteresting(struct rev_info *revs,
if (sparse) {
struct tree *tree = get_commit_tree(commit);
+ if (!tree)
+ continue;
if (commit->object.flags & UNINTERESTING)
tree->object.flags |= UNINTERESTING;
I will definitely include a GIT_TEST_* variable in v2.
Thanks,
-Stolee
^ permalink raw reply related
* Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
From: Junio C Hamano @ 2018-11-29 2:59 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Eric Sunshine
In-Reply-To: <20181128201852.9782-3-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> + [--range-diff<common diff option>]]
Let's make sure a random string thrown at this mechanism will
properly get noticed and diagnosed.
> @@ -257,6 +258,13 @@ feeding the result to `git send-email`.
> creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
> for details.
>
> +--range-diff<common diff option>::
> + Other options prefixed with `--range-diff` are stripped of
> + that prefix and passed as-is to the diff machinery used to
> + generate the range-diff, e.g. `--range-diff-U0` and
> + `--range-diff--no-color`. This allows for adjusting the format
> + of the range-diff independently from the patch itself.
Taking anything is of course the most general, but I am afraid if
this backfires if there are some options that do not make sense to
be different between the invocations of range-diff and format-patch.
> @@ -1689,8 +1688,32 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> rev.preserve_subject = keep_subject;
>
> argc = setup_revisions(argc, argv, &rev, &s_r_opt);
> - if (argc > 1)
> - die(_("unrecognized argument: %s"), argv[1]);
> + if (argc > 1) {
> + struct argv_array args = ARGV_ARRAY_INIT;
> + const char *prefix = "--range-diff";
Please call that anything but "prefix" that hides the parameter to
the function.
const char *range_diff_opt = "--range-diff";
might work OK, or it might not. Let's read on.
> + int have_prefix = 0;
> +
> + for (i = 0; i < argc; i++) {
> + struct strbuf sb = STRBUF_INIT;
> + char *str;
> +
> + strbuf_addstr(&sb, argv[i]);
> + if (starts_with(argv[i], prefix)) {
> + have_prefix = 1;
> + strbuf_remove(&sb, 0, strlen(prefix));
> + }
> + str = strbuf_detach(&sb, NULL);
> + strbuf_release(&sb);
> +
> + argv_array_push(&args, str);
> + }
> +
Is the body of the loop essentially this?
char *passopt = argv[i];
if (!skip_prefix(passopt, range_diff_opt, &passopt))
saw_range_diff_opt = 1;
argv_array_push(&args, xstrdup(passopt));
We only use that "prefix" thing once, so we may not even need the
variable.
> + if (!have_prefix)
> + die(_("unrecognized argument: %s"), argv[1]);
So we take normal options and check the leftover args; if there is
no --range-diff<whatever> among the leftover bits, we pretend that
we stumbled while reading the first such leftover arg.
> + argc = setup_revisions(args.argc, args.argv, &rd_rev, NULL);
> + if (argc > 1)
> + die(_("unrecognized argument: %s"), argv[1]);
> + }
Otherwise, we pass all the leftover bits, which is a random mixture
but guaranteed to have at least one meant for range-diff, to another
setup_revisions(). If it leaves a leftover arg, then that is
diagnosed here, so we'd be OK (iow, this is not a new "attack
vector" to inject random string to command line parser).
One minor glitch I can see is "format-patch --range-diffSilly" would
report "unrecognised arg: Silly". As we are pretending to be and
reporting errors as format-patch, it would be better if we report
that --range-diffSilly was what we did not understand.
> Junio: I know it's late, but unless Eric has objections to this UI
> change I'd really like to have this in 2.20 since this is a change to
> a new command-line UI that's newly added in 2.20.
Quite honestly, I'd rather document "driving range-diff from
format-patch is experimental and does silly things when given
non-standard options in this release" and not touch the code at this
late stage in the game. Would it be less intrusive a change to
*not* support the --range-diff<whatever> option, still use rd_rev
that is separate from the main rev, and use a reasonable hardcoded
default settings when preparing rd_rev?
^ permalink raw reply
* Re: Forcing GC to always fail
From: Bryan Turner @ 2018-11-29 2:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: newren, Ævar Arnfjörð Bjarmason, Git Users
In-Reply-To: <xmqqtvk0r87v.fsf@gitster-ct.c.googlers.com>
On Wed, Nov 28, 2018 at 5:19 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> > Another issue with the canned steps for "git gc" is that it means it
> > can't be used to do specific types of cleanup on a different schedule
> > from others. For example, we use "git pack-refs" directly to
> > frequently pack the refs in our repositories, separate from "git
> > repack" + "git prune" for repacking objects. That allows us to keep
> > our refs packed better without incurring the full overhead of
> > constantly building new packs.
>
> I am not sure if the above is an example of things that are good.
> We keep individual "pack-refs" and "rev-list | pack-objects"
> available exactly to give finer grained control to repository
> owners, and "gc" is meant to be one-size-fits-all easy to run
> by end users. Adding options to "git gc --no-reflog --pack-refs"
> to complicate it sounds somewhat backwards.
I think we're in agreement there. I was citing the fact that GC isn't
good for targeted maintenance as a reason why we use "pack-refs"
directly, which sounds like what you're saying as well. I don't think
that inflating GC with options to skip specific steps is a good idea,
but that does mean that, for those targeted operations, we need to use
the lower-level commands directly, rather than GC.
Bryan
^ permalink raw reply
* Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
From: Junio C Hamano @ 2018-11-29 2:11 UTC (permalink / raw)
To: Johannes Sixt; +Cc: brian m. carlson, Frank Schäfer, git
In-Reply-To: <3e24a770-47fc-50e4-d757-1e4a28dcd019@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 27.11.18 um 00:31 schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>> Am 26.11.18 um 04:04 schrieb Junio C Hamano:
>>> ... this goes too far, IMO. It is the pager's task to decode control
>>> characters.
>>
>> It was tongue-in-cheek suggestion to split a CR into caret-em on our
>> end, but we'd get essentially the same visual effect if we added a
>> rule:
>>
>> When producing a colored output (not limited to whitespace
>> error coloring of diff output), insert <RESET> before a CR
>> that comes immediately before a LF.
This was misspoken a bit. Let's revise it to
When producing a colored output (not limited to whitespace
error coloring of diff output) for contents that are not
marked as eol=crlf (and other historical means), insert
<RESET> before a CR that comes immediately before a LF.
to exempt CR in CRLF sequence that the contents and the user agree
to be part of the end-of-line marker.
> I wouldn't want that to happen for all output (context lines, - lines,
> + lines): I really am not interested to see all the CRs in my CRLF
> files.
So your CRLF files will not give caret-em.
>> And a good thing is that I do not think that new rule is doing any
>> decode of control chars on our end. We are just producing colored
>> output normally.
>
> But we already have it, as Brian pointed out:
>
> git diff --ws-error-highlight=old,new
>
> or by setting diff.wsErrorHighlight accordingly.
Not really. If the context lines end with CRLF and the material is
not CRLF, I do not think CR at the end of them should be highlighted
as whitespace errors (as we tell to detect errors on "old" and "new"
lines), but I think we still should make an effort to help them be
visible to the users. Otherwise, next Frank will come and complain
after seeing
-something
some common thing
+something_new^M
With the suggested change, you can distinguish the following two
(and use your imagination that there are many "some common thing"
lines), which would help the user guide if the file should be marked
as CRLF file, or the contents mistakenly has some lines that end
with CRLF by mistake. The corrective action between the two cases
would vastly be different.
-something^M -something
some common thing^M some common thing
some common thing^M some common thing
some common thing^M some common thing
+something_new^M +something_new^M
Without, both would look like the RHS of the illustration, and with
the "highlight both", you'd only get an extra caret-em on the
removed line, and need to judge without the help from common context
lines.
Besides, --ws-error-highlight shows all whitespace errors, and
showing CR as caret-em is mere side effect of the interaction
between its coloring and the pager.
^ permalink raw reply
* Re: in 2.19.2 t0061-run-command FAILs if . is in $PATH
From: Junio C Hamano @ 2018-11-29 1:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: H.Merijn Brand, git
In-Reply-To: <nycvar.QRO.7.76.6.1811281041400.41@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> -test_expect_success 'run_command is restricted to PATH' '
> +test_lazy_prereq DOT_IN_PATH '
> + case ":$PATH:" in
> + *:.:*) true;;
> + *) false;;
> + esac
> +'
An empty element in the colon-separated list also serves as an
instruction to pick up executable from $cwd, so
case ":$PATH:" in
*:.:** | *::*) true ;;
*) false ;;
esac
perhaps.
> +test_expect_success !DOT_IN_PATH 'run_command is restricted to PATH' '
> write_script should-not-run <<-\EOF &&
> echo yikes
> EOF
> -- snap --
>
> If so, can you please provide a commit message for it (you can add my
> Signed-off-by: line and your Tested-by: line).
>
> Thanks,
> Johannes
^ permalink raw reply
* Re: Forcing GC to always fail
From: Junio C Hamano @ 2018-11-29 1:19 UTC (permalink / raw)
To: Bryan Turner; +Cc: newren, Ævar Arnfjörð Bjarmason, Git Users
In-Reply-To: <CAGyf7-FwGRVtFYbOGW8DmY9F-cgun0n5K-ZWBjXCR=wvWh=8dQ@mail.gmail.com>
Bryan Turner <bturner@atlassian.com> writes:
> For us, the biggest issue was "git gc"'s insistence on trying to run
> "git reflog expire". That triggers locking behaviors that resulted in
> very frequent GC failures--and the only reflogs Bitbucket Server (by
> default) creates are all configured to never ex[ire or be pruned, so
> the effort is all wasted anyway.
Detecting that the expiry threshold is set to "never" before
spending cycles and seeks to sift between old and new and not
spawning the expire command?
This seems like an obvious low-hanging fruit to me.
> Another issue with the canned steps for "git gc" is that it means it
> can't be used to do specific types of cleanup on a different schedule
> from others. For example, we use "git pack-refs" directly to
> frequently pack the refs in our repositories, separate from "git
> repack" + "git prune" for repacking objects. That allows us to keep
> our refs packed better without incurring the full overhead of
> constantly building new packs.
I am not sure if the above is an example of things that are good.
We keep individual "pack-refs" and "rev-list | pack-objects"
available exactly to give finer grained control to repository
owners, and "gc" is meant to be one-size-fits-all easy to run
by end users. Adding options to "git gc --no-reflog --pack-refs"
to complicate it sounds somewhat backwards.
^ permalink raw reply
* Re: [PATCH 09/10] fetch: try fetching submodules if needed objects were not fetched
From: Stefan Beller @ 2018-11-29 0:30 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <20181026204106.132296-1-jonathantanmy@google.com>
On Fri, Oct 26, 2018 at 1:41 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > But this default fetch is not sufficient, as a newly fetched commit in
> > the superproject could point to a commit in the submodule that is not
> > in the default refspec. This is common in workflows like Gerrit's.
> > When fetching a Gerrit change under review (from refs/changes/??), the
> > commits in that change likely point to submodule commits that have not
> > been merged to a branch yet.
> >
> > Try fetching a submodule by object id if the object id that the
> > superproject points to, cannot be found.
>
> I see that these suggestions of mine (from [1]) was implemented, but not
> others. If you disagree, that's fine, but I think they should be
> discussed.
ok.
>
> > - if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
> > - (recurse_submodules != RECURSE_SUBMODULES_ON))
> > + if (recurse_submodules != RECURSE_SUBMODULES_OFF)
>
> I think the next patch should be squashed into this patch. Then you can
> say that these are now redundant and can be removed.
ok.
>
> > @@ -1218,8 +1218,12 @@ struct submodule_parallel_fetch {
> > int result;
> >
> > struct string_list changed_submodule_names;
> > + struct get_next_submodule_task **fetch_specific_oids;
> > + int fetch_specific_oids_nr, fetch_specific_oids_alloc;
> > };
>
> Add documentation for fetch_specific_oids. Also, it might be better to
> call these oid_fetch_tasks and the struct, "struct fetch_task".
ok.
> Here, struct get_next_submodule_task is used for 2 different things:
> (1) After the first round fetch, fetch_finish() uses it to determine if
> a second round is needed.
> (2) In submodule_parallel_fetch.fetch_specific_oids, to tell the
> parallel runner (through get_next_submodule_task()) to do this
> fetch.
>
> I think that it's better to have 2 different structs for these 2
> different uses. (Note that task_cb can be NULL for the second round.
> Unless we plan to recheck the OIDs? Currently we recheck them, but we
> don't do anything either way.)
I think it is easier to only have one struct until we have substantially
more to communicate. (1) is a boolean answer, for which (non-)NULL
is sufficient.
> I think that this is best described as the submodule that has no entry
> in .gitmodules? Maybe call it "get_non_gitmodules_submodule" and
> document it with a similar comment as in get_submodule_repo_for().
done.
>
> > +
> > +static struct get_next_submodule_task *get_next_submodule_task_create(
> > + struct repository *r, const char *path)
> > +{
> > + struct get_next_submodule_task *task = xmalloc(sizeof(*task));
> > + memset(task, 0, sizeof(*task));
> > +
> > + task->sub = submodule_from_path(r, &null_oid, path);
> > + if (!task->sub) {
> > + task->sub = get_default_submodule(path);
> > + task->free_sub = 1;
> > + }
> > +
> > + return task;
> > +}
>
> Clearer to me would be to make get_next_submodule_task_create() return
> NULL if submodule_from_path() returns NULL.
I doubled down on this one and return NULL when get_default_submodule
(now renamed to get_non_gitmodules_submodule) returns NULL, as then we
can move the free() from get_next_submodule here and there we'll just have
task = fetch_task_create(spf->r, ce->name);
if (!task)
continue;
which helps get_next_submodule to stay readable.
> Same comment about "on-demand" as in my previous e-mail.
I'd want to push back on refactoring and defer that to a later series.
> Break lines to 80.
[...]
> Same comment about "s--h" as in my previous e-mail.
done
> > + /* Are there commits that do not exist? */
> > + if (commits->nr) {
> > + /* We already tried fetching them, do not try again. */
> > + if (task->commits)
> > + return 0;
>
> Same comment about "task->commits" as in my previous e-mail.
Good call. I reordered the function read easier and added a comment
on any early return how it could happen.
>
> > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> > index 6c2f9b2ba2..5a75b57852 100755
>
> One more thing to test is the case where a submodule doesn't have a
> .gitmodules entry.
added a test.
I just resent the series.
Stefan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox