* Re: [PATCH v2 1/2] Change how HTTP response body is returned
From: Jeff King @ 2019-01-04 10:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Masaya Suzuki, git, jrnieder, sunshine
In-Reply-To: <xmqqtviph835.fsf@gitster-ct.c.googlers.com>
On Thu, Jan 03, 2019 at 11:09:02AM -0800, Junio C Hamano wrote:
> > + if (dest->file) {
> > + /*
> > + * At this point, the file contains the response body of the
> > + * previous request. We need to truncate the file.
> > + */
> > + FILE *new_file = freopen(dest->filename, "w", dest->file);
>
> Now freopen() lets us restart the file anew with a new "FILE *".
>
> > + if (new_file == NULL) {
> > + error("Unable to open local file %s", dest->filename);
>
> error_errno(), perhaps?
>
> At this point, I presume that dest->file is closed by the failed
> freopen(), but dest->file is still non-NULL and causes further calls
> to http_request() with this dest would be a disaster? As long as
> the caller of this function reacts to HTTP_ERROR and kill the dest,
> it would be fine.
I also wondered what timing guarantees freopen() gives us (i.e., is it
possible for it to open and truncate the file, and then close the old
handle, flushing some in-memory buffer). C99 says:
The freopen function first attempts to close any file that is
associated with the specified stream. Failure to close the file is
ignored. The error and end-of-file indicators for the stream are
cleared.
So I think the order is OK for my concern, but not for yours. I.e., on
an error, dest->file is now undefined.
It might be nice to set "dest->file == NULL" in that case. There's no
guarantee that the caller did not hold onto its own copy of the handle,
but since this struct is only exposed internally within http.c, that's
probably OK.
The most robust thing would perhaps be:
fflush(dest->file);
ftruncate(fileno(dest->file), 0);
which leaves the handle intact.
(I agree with the rest of your review, especially that it would be
easier to read if this were split into separate refactor and
change-behavior steps).
-Peff
^ permalink raw reply
* Re: [PATCH v2 1/2] Change how HTTP response body is returned
From: Jeff King @ 2019-01-04 10:30 UTC (permalink / raw)
To: Masaya Suzuki; +Cc: git, jrnieder, sunshine
In-Reply-To: <20181229194447.157763-2-masayasuzuki@google.com>
On Sat, Dec 29, 2018 at 11:44:46AM -0800, Masaya Suzuki wrote:
> +/*
> + * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
> + * from ptr.
> + */
> static size_t rpc_in(char *ptr, size_t eltsize,
> size_t nmemb, void *buffer_)
> {
> size_t size = eltsize * nmemb;
> - struct rpc_state *rpc = buffer_;
> + struct rpc_in_data *data = buffer_;
> + long response_code;
> +
> + if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
> + &response_code) != CURLE_OK)
> + return size;
This hunk was unexpected to me. The function here is just writing out
the data, and I expected we'd handle the error after the whole transfer
is done. But we do that anyway eventually via run_slot() (which uses
handle_curl_result). I guess the goal here is to start throwing away
data when we see an error, rather than storing it?
That makes some sense, though I do wonder if there's any case where curl
would call our WRITEFUNCTION before it knows the HTTP status. That
implies a body before our header, which seems impossible, though.
> + if (response_code != 200)
> + return size;
The current behavior with CURLOPT_FAILONERROR treats codes >= 400 as an
error. And in handle_curl_result(), we treat >= 300 as an error (since
we only see 3xx for a disabled redirect). I suppose it's unlikely for us
to see any success code besides 200, but we probably ought to be
following the same rules here.
-Peff
^ permalink raw reply
* Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR
From: Jeff King @ 2019-01-04 10:49 UTC (permalink / raw)
To: Masaya Suzuki; +Cc: git, jrnieder, sunshine
In-Reply-To: <20181229194447.157763-3-masayasuzuki@google.com>
On Sat, Dec 29, 2018 at 11:44:47AM -0800, Masaya Suzuki wrote:
> When GIT_CURL_VERBOSE is set, libcurl produces request/response headers
> to stderr. However, if the response is an error response and
> CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it
> won't dump the headers. Showing HTTP response headers is useful for
> debugging, especially for non-OK responses.
Out of curiosity, does GIT_TRACE_CURL do any better? Or is it simply
that curl closes the handle when it sees the bad response code, and
nobody ever gets to see the rest of the data?
> This is substantially same as setting http_options.keep_error to all
> requests. Hence, remove this option.
The assumption here is that every code path using FAILONERROR is
prepared to handle the failing http response codes itself (since we no
longer set it at all in get_active_slot()). Is that so?
Anything that uses handle_curl_result() is OK. That means run_one_slot()
is fine, which in turn covers run_slot() for RPCs, and http_request()
for normal one-at-a-time requests. But what about the parallel multiple
requests issued by the dumb-http walker code?
There I think we end up in step_active_slots(), which calls into
finish_active_slot() for completed requests. I think that
unconditionally fetches the http code without bothering to look at
whether curl reported success or not.
So I _think_ that's probably all of the users of the curl handles
provided by get_active_slot(). Though given the tangled mess of our HTTP
code, I won't be surprised if there's a corner case I missed in that
analysis.
-Peff
^ permalink raw reply
* Re: [PATCH v2] test-lib: check Bash version for '-x' without using shell arrays
From: Junio C Hamano @ 2019-01-04 11:25 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Johannes Sixt, Max Kirillov, Carlo Arenas, Jeff King,
Eric Sunshine, git
In-Reply-To: <20190104093015.GC4673@szeder.dev>
SZEDER Gábor <szeder.dev@gmail.com> writes:
> On Thu, Jan 03, 2019 at 12:29:35PM -0800, Junio C Hamano wrote:
>> Let's treat this as an independent and more urgent fix-up. I think
>> it is sufficient to apply it to 2.20.x track, even though we could
>> go back to 2.17.x and above.
>>
>> And then let's tentatively kick the "stress test" series out of
>> 'pu', and have that series rebuilt on top of 'master' and this
>> patch.
>
> I rebased my '--stress' patch series on top of
> 'sg/test-bash-version-fix', and the result is the same as what's at
> the tip of 'sg/stress-test' at 1d1416a34b.
Yeah, sorry for not reporting what I did after pushing the
integration result out. I ended up rebasing the stress series on
top of the updated fix with eval myself.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] test-lib: check Bash version for '-x' without using shell arrays
From: SZEDER Gábor @ 2019-01-04 12:38 UTC (permalink / raw)
To: Junio C Hamano, Carlo Arenas
Cc: Johannes Sixt, Max Kirillov, Jeff King, Eric Sunshine, git
In-Reply-To: <xmqq5zv4fyuy.fsf@gitster-ct.c.googlers.com>
On Fri, Jan 04, 2019 at 03:25:57AM -0800, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
> > On Thu, Jan 03, 2019 at 12:29:35PM -0800, Junio C Hamano wrote:
> >> Let's treat this as an independent and more urgent fix-up. I think
> >> it is sufficient to apply it to 2.20.x track, even though we could
> >> go back to 2.17.x and above.
> >>
> >> And then let's tentatively kick the "stress test" series out of
> >> 'pu', and have that series rebuilt on top of 'master' and this
> >> patch.
> >
> > I rebased my '--stress' patch series on top of
> > 'sg/test-bash-version-fix', and the result is the same as what's at
> > the tip of 'sg/stress-test' at 1d1416a34b.
>
> Yeah, sorry for not reporting what I did after pushing the
> integration result out. I ended up rebasing the stress series on
> top of the updated fix with eval myself.
No problem, it still saved me from writing a cover letter :)
Note that I didn't actually test either of these patches on NetBSD,
though both "Should Work". Carlo sent a Tested-by for v1; I hope
he'll be able to get around and test v2 as well.
^ permalink raw reply
* Re: [PATCH 1/3] Add 'human' date format
From: Stephen P Smith @ 2019-01-04 13:03 UTC (permalink / raw)
To: Jeff King
Cc: git, Linus Torvalds, Junio C Hamano,
Ævar Arnfjörð Bjarmason
In-Reply-To: <20190104075034.GA26014@sigill.intra.peff.net>
On Friday, January 4, 2019 12:50:35 AM MST Jeff King wrote:
> On Thu, Jan 03, 2019 at 06:19:56AM -0700, Stephen P. Smith wrote:
> >
> > I didn't see anything in the code which would prohibit setting something
> > like that.
>
> Yeah, I don't think supporting that is too hard. I was thinking
> something like this:
I take it that if I update Linus's patch, I still keep Junio's and Linus'
sign-off line for the purpose of the chain of custody? Of should I use a
second patch?
Just trying to follow the rules.
sps
^ permalink raw reply
* [PATCH 0/1] i18n: add framework for localizing the manpages
From: Jean-Noël Avila @ 2019-01-04 16:54 UTC (permalink / raw)
To: git; +Cc: Jean-Noël Avila
Hi all,
This is a second attempt at providing localized manpages of git in a central way. The first attempt[1] was to include all the changes directly in the main repo. But as Junio made me realize, staying in the main repo would have many drawbacks such as forcing possible translators to follow the workflow of Git and preventing use of third party translation tools such as Weblate.
This solution is to make the manpage localization project standalone. I finally had time to hack po4a to enhance support for specificities of git manpages. This rather light patch is only an adaptation of the Makefile to be able to use the same building rules in the translation repo. The translation repo bases the workflow on a copy of the selected manpages asciidoc source files. You can find it at
https://github.com/jnavila/git-manpages-l10n
Translations can be filled using Weblate at https://hosted.weblate.org/projects/git-manpages/
[1] https://public-inbox.org/git/20170312200248.3610-1-jn.avila@free.fr/
Jean-Noel Avila (1):
Add optional targets for documentation l10n
Documentation/Makefile | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH 1/1] Add optional targets for documentation l10n
From: Jean-Noël Avila @ 2019-01-04 16:54 UTC (permalink / raw)
To: git; +Cc: Jean-Noel Avila
In-Reply-To: <20190104165406.22358-1-jn.avila@free.fr>
From: Jean-Noel Avila <jn.avila@free.fr>
The standard doc lists can be filtered to allow using the compilation
rules with translated manpages where all the pages of the original
version may not be present.
The install variable are reused in the secondary repo so that the
configured paths can be used for translated manpages too.
Signed-off-by: Jean-Noël Avila <jn.avila@free.fr>
---
Documentation/Makefile | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index b5be2e2d3f..1f61a1fe86 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -35,13 +35,18 @@ MAN7_TXT += gittutorial-2.txt
MAN7_TXT += gittutorial.txt
MAN7_TXT += gitworkflows.txt
-MAN_TXT = $(MAN1_TXT) $(MAN5_TXT) $(MAN7_TXT)
+TMP_MAN_TXT = $(MAN1_TXT) $(MAN5_TXT) $(MAN7_TXT)
+MAN_FILTER ?= $(TMP_MAN_TXT)
+MAN_TXT = $(filter $(TMP_MAN_TXT), $(MAN_FILTER))
+undefine TMP_MAN_TXT
+
MAN_XML = $(patsubst %.txt,%.xml,$(MAN_TXT))
MAN_HTML = $(patsubst %.txt,%.html,$(MAN_TXT))
OBSOLETE_HTML += everyday.html
OBSOLETE_HTML += git-remote-helpers.html
-DOC_HTML = $(MAN_HTML) $(OBSOLETE_HTML)
+
+TMP_DOC_HTML = $(MAN_HTML) $(OBSOLETE_HTML)
ARTICLES += howto-index
ARTICLES += git-tools
@@ -81,11 +86,14 @@ TECH_DOCS += technical/trivial-merge
SP_ARTICLES += $(TECH_DOCS)
SP_ARTICLES += technical/api-index
-DOC_HTML += $(patsubst %,%.html,$(ARTICLES) $(SP_ARTICLES))
+TMP_DOC_HTML += $(patsubst %,%.html,$(ARTICLES) $(SP_ARTICLES))
+HTML_FILTER ?= $(TMP_DOC_HTML)
+DOC_HTML = $(filter $(HTML_FILTER),$(TMP_DOC_HTML))
+undefine TMP_DOC_HTML
-DOC_MAN1 = $(patsubst %.txt,%.1,$(MAN1_TXT))
-DOC_MAN5 = $(patsubst %.txt,%.5,$(MAN5_TXT))
-DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
+DOC_MAN1 = $(patsubst %.txt,%.1,$(filter $(MAN_FILTER), $(MAN1_TXT)))
+DOC_MAN5 = $(patsubst %.txt,%.5,$(filter $(MAN_FILTER), $(MAN5_TXT)))
+DOC_MAN7 = $(patsubst %.txt,%.7,$(filter $(MAN_FILTER), $(MAN7_TXT)))
prefix ?= $(HOME)
bindir ?= $(prefix)/bin
@@ -444,4 +452,9 @@ print-man1:
lint-docs::
$(QUIET_LINT)$(PERL_PATH) lint-gitlink.perl
+ifeq ($(wildcard po/Makefile),po/Makefile)
+doc-l10n install-l10n::
+ $(MAKE) -C po $@
+endif
+
.PHONY: FORCE
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 4/4] built-in rebase: call `git am` directly
From: Junio C Hamano @ 2019-01-04 18:38 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Johannes Schindelin, Elijah Newren, Orgad Shaneh
In-Reply-To: <2b5ece8263936f0a7dfad864c0de43d784fdaf1f.1545398254.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> +static int write_basic_state(struct rebase_options *opts)
> +{
> + write_file(state_dir_path("head-name", opts), "%s",
> + opts->head_name ? opts->head_name : "detached HEAD");
> + write_file(state_dir_path("onto", opts), "%s",
> + opts->onto ? oid_to_hex(&opts->onto->object.oid) : "");
> + write_file(state_dir_path("orig-head", opts), "%s",
> + oid_to_hex(&opts->orig_head));
> + write_file(state_dir_path("quiet", opts), "%s",
> + opts->flags & REBASE_NO_QUIET ? "" : "t");
> + if (opts->flags & REBASE_VERBOSE)
> + write_file(state_dir_path("verbose", opts), "%s", "");
> + if (opts->strategy)
> + write_file(state_dir_path("strategy", opts), "%s",
> + opts->strategy);
> + if (opts->strategy_opts)
> + write_file(state_dir_path("strategy_opts", opts), "%s",
> + opts->strategy_opts);
> + if (opts->allow_rerere_autoupdate >= 0)
> + write_file(state_dir_path("allow_rerere_autoupdate", opts),
> + "-%s-rerere-autoupdate",
> + opts->allow_rerere_autoupdate ? "" : "-no");
Inside rebase, allow-rerere-autoupdate can be -1 (unspecified), 0
(declined) or 1 (requested), and this code is being consistent with
that convention.
The "--[no-]rerere-autoupdate" option that is parsed via
OPT_RERERE_AUTOUPDATE (used in builtin/rebase--interactive.c among
other built-in commands) on the other hand is tertially that uses 0
(unspecified), 1 (requested) and 2 (declined). This might be a
ticking timebomb to confuse us in the future that may be worth
fixing but probably outside this series.
> + if (opts->gpg_sign_opt)
> + write_file(state_dir_path("gpg_sign_opt", opts), "%s",
> + opts->gpg_sign_opt);
> + if (opts->signoff)
> + write_file(state_dir_path("strategy", opts), "--signoff");
> +
> + return 0;
> +}
The above looks like a literal translation of a helper function with
the same name in git-rebase--common.sh. Good.
> @@ -459,6 +490,30 @@ static int reset_head(struct object_id *oid, const char *action,
> return ret;
> }
>
> +static int move_to_original_branch(struct rebase_options *opts)
> +{
> + struct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
> + int ret;
> +
> + if (!opts->head_name)
> + return 0; /* nothing to move back to */
> +
> + if (!opts->onto)
> + BUG("move_to_original_branch without onto");
This check is absent in the scripted version, but from the message
we generate here, it is clear that the caller must not call this
when there is no "onto" commit. Good.
> + strbuf_addf(&orig_head_reflog, "rebase finished: %s onto %s",
> + opts->head_name, oid_to_hex(&opts->onto->object.oid));
> + strbuf_addf(&head_reflog, "rebase finished: returning to %s",
> + opts->head_name);
> + ret = reset_head(NULL, "checkout", opts->head_name,
> + RESET_HEAD_REFS_ONLY,
> + orig_head_reflog.buf, head_reflog.buf);
The *action given to reset_head() here is "checkout". Makes me
wonder about two things:
- The only real use of the parameter in the callee is to prepare
the error and advice messages from the unpack_trees machinery,
but because we are using it in REFS_ONLY mode, it does not
matter. In fact it might even be misleading; perhaps pass NULL
or something, so that a mistaken update to reset_head() later
that lets REFS_ONLY request to go to unpack_trees machinery will
catch it as a bug?
- Another topic in flight wants to make sure that the post-checkout
hook gets called when the RESET_HEAD_RUN_POST_CHECKOUT_HOOK flag
is given by the caller, and IIRC, the use of the flag is strongly
correlated to *action being "checkout". Do we want to pass
REFS_ONLY and RUN_POST_CHECKOUT_HOOK flag for this call, or do we
rather keep it silent? As the original scripted version did not
use "checkout" here and never triggered post-checkout hook, I am
inclined to say that we should not pass that other bit. That
then leads me to suspect that we do not want *action to be
"checkout" here.
> + strbuf_release(&orig_head_reflog);
> + strbuf_release(&head_reflog);
> + return ret;
> +}
Unlike the scripted version, this does not die() upon failure, so
the caller needs to be careful about the returned status.
> @@ -466,6 +521,129 @@ N_("Resolve all conflicts manually, mark them as resolved with\n"
> "To abort and get back to the state before \"git rebase\", run "
> "\"git rebase --abort\".");
>
> +static int run_am(struct rebase_options *opts)
> +{
> + struct child_process am = CHILD_PROCESS_INIT;
> + struct child_process format_patch = CHILD_PROCESS_INIT;
> + struct strbuf revisions = STRBUF_INIT;
> + int status;
> + char *rebased_patches;
> +
> + am.git_cmd = 1;
> + argv_array_push(&am.args, "am");
> +
> + if (opts->action && !strcmp("continue", opts->action)) {
> + argv_array_push(&am.args, "--resolved");
> + argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
> + if (opts->gpg_sign_opt)
> + argv_array_push(&am.args, opts->gpg_sign_opt);
> + status = run_command(&am);
> + if (status)
> + return status;
> +
> + discard_cache();
> + return move_to_original_branch(opts);
It is curious why discard_cache() is placed exacly here, as if we
want to preserve the contents of the in-core index when
run_command() failed. But I do not think we care about the in-core
index as the only thing that happen after "return status" is to
return the control to run_specific_rebase(), let it jump to
finished_rebase label to clean things up and rturn control to
cmd_rebase() and exit based on the status value.
It's not like move_to_original_branch() wants to call read_cache()
and get the result from the "am" that run_command() executed,
either.
Puzzled. Care to explain a bit more in the in-code comment?
> + }
> + if (opts->action && !strcmp("skip", opts->action)) {
> + argv_array_push(&am.args, "--skip");
> + argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
> + status = run_command(&am);
> + if (status)
> + return status;
> +
> + discard_cache();
> + return move_to_original_branch(opts);
Ditto.
> + }
> + if (opts->action && !strcmp("show-current-patch", opts->action)) {
> + argv_array_push(&am.args, "--show-current-patch");
> + return run_command(&am);
> + }
Up to this point, it is a faithful conversion of the first case/esac
statement. Good.
> + strbuf_addf(&revisions, "%s...%s",
> + oid_to_hex(opts->root ?
> + /* this is now equivalent to ! -z "$upstream" */
Does "this" refer to the "opts->root being true" check?
Because you are flipping the polarity of the test from scripted
version, shouldn't the comment be updated to "-z $upstream"?
> + &opts->onto->object.oid :
> + &opts->upstream->object.oid),
> + oid_to_hex(&opts->orig_head));
> + rebased_patches = xstrdup(git_path("rebased-patches"));
> + format_patch.out = open(rebased_patches,
> + O_WRONLY | O_CREAT | O_TRUNC, 0666);
Unlike scripted version, we do not remove a (possibly) existing file.
We give CREAT in case there is no existing one, and TRUNC in case
there is an existing one. Makes sense. A more faithful translation
would have unlink(2)ed a (possibly) existing one, and then because
we can afford to, passed O_EXCL to avoid stomping on somebody else
racing with us, but I do not think it is worth it.
> + if (format_patch.out < 0) {
> + status = error_errno(_("could not write '%s'"),
> + rebased_patches);
s/write '%s'/open '%s' for writing/? I dunno.
> + free(rebased_patches);
> + argv_array_clear(&am.args);
> + return status;
> + }
> +
> + format_patch.git_cmd = 1;
> + argv_array_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
> + "--full-index", "--cherry-pick", "--right-only",
> + "--src-prefix=a/", "--dst-prefix=b/", "--no-renames",
> + "--no-cover-letter", "--pretty=mboxrd", NULL);
> + if (opts->git_format_patch_opt.len)
> + argv_array_split(&format_patch.args,
> + opts->git_format_patch_opt.buf);
> + argv_array_push(&format_patch.args, revisions.buf);
> + if (opts->restrict_revision)
> + argv_array_pushf(&format_patch.args, "^%s",
> + oid_to_hex(&opts->restrict_revision->object.oid));
It is kinda surprising to see that we have learned quite a lot of
fringe "configurations" we need to explicitly override like this.
Looks like a quite faithful conversion, anyway.
> + status = run_command(&format_patch);
> + if (status) {
> + unlink(rebased_patches);
> + free(rebased_patches);
> + argv_array_clear(&am.args);
> +
> + reset_head(&opts->orig_head, "checkout", opts->head_name, 0,
> + "HEAD", NULL);
This one may need to trigger post-checkout hook. The scripted
version does two different things depending on the value of
$head_name, but we can just use the same code without conditional?
> + error(_("\ngit encountered an error while preparing the "
> + "patches to replay\n"
> + "these revisions:\n"
> + "\n %s\n\n"
> + "As a result, git cannot rebase them."),
> + opts->revisions);
> +
> + strbuf_release(&revisions);
> + return status;
> + }
> + strbuf_release(&revisions);
> +
> + am.in = open(rebased_patches, O_RDONLY);
> + if (am.in < 0) {
> + status = error_errno(_("could not read '%s'"),
> + rebased_patches);
s/write '%s'/open '%s' for reading/? I dunno.
> + free(rebased_patches);
> + argv_array_clear(&am.args);
> + return status;
> + }
> +
> + argv_array_pushv(&am.args, opts->git_am_opts.argv);
> + argv_array_push(&am.args, "--rebasing");
> + argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
> + argv_array_push(&am.args, "--patch-format=mboxrd");
> + if (opts->allow_rerere_autoupdate > 0)
> + argv_array_push(&am.args, "--rerere-autoupdate");
> + else if (opts->allow_rerere_autoupdate == 0)
> + argv_array_push(&am.args, "--no-rerere-autoupdate");
> + if (opts->gpg_sign_opt)
> + argv_array_push(&am.args, opts->gpg_sign_opt);
> + status = run_command(&am);
> + unlink(rebased_patches);
> + free(rebased_patches);
> +
> + if (!status) {
> + discard_cache();
> + return move_to_original_branch(opts);
> + }
> +
> + if (is_directory(opts->state_dir))
> + write_basic_state(opts);
> +
> + return status;
> +}
> +
> static int run_specific_rebase(struct rebase_options *opts)
> {
> const char *argv[] = { NULL, NULL };
> @@ -546,6 +724,11 @@ static int run_specific_rebase(struct rebase_options *opts)
> goto finished_rebase;
> }
>
> + if (opts->type == REBASE_AM) {
> + status = run_am(opts);
> + goto finished_rebase;
> + }
> +
> add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
> add_var(&script_snippet, "state_dir", opts->state_dir);
Overall, this was quite a pleasant read and a well constructed
series. Other than two minor points (i.e. interaction with the
'post-checkout hook' topic, and discard_cache() before calling
move_to_original_branch) I did not quite understand, looks good to
me.
When merged to 'pu', I seem to be getting failure from t3425.5, .8
and .11, by the way. I haven't dug into the actual breakages any
further than that.
Thanks.
^ permalink raw reply
* Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree
From: Junio C Hamano @ 2019-01-04 18:38 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <21939140e00d96cf6f76e4c994638fecd3a95639.1545398254.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> This is what the legacy (scripted) rebase does in
> `move_to_original_branch`, and we will need this functionality in the
> next commit.
The move-to-original-branch helper does:
- point $head_name to the commit pointed at by HEAD
- point HEAD symref to $head_name
without touching the index or the working tree files. It's not
exactly "reset --soft" but more like "switch-branch --soft" ;-)
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/rebase.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 768bea0da8..303175fdf1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
>
> #define RESET_HEAD_DETACH (1<<0)
> #define RESET_HEAD_HARD (1<<1)
> +#define RESET_HEAD_REFS_ONLY (1<<2)
In the future codebase in 'pu', we have 1<<2 already taken by
another topic, so I'll tell my rerere database that the bit
assignment needs to be adjusted.
> static int reset_head(struct object_id *oid, const char *action,
> const char *switch_to_branch, unsigned flags,
> @@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action,
> {
> unsigned detach_head = flags & RESET_HEAD_DETACH;
> unsigned reset_hard = flags & RESET_HEAD_HARD;
> + unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
> struct object_id head_oid;
> struct tree_desc desc[2] = { { NULL }, { NULL } };
> struct lock_file lock = LOCK_INIT;
> @@ -359,7 +361,7 @@ static int reset_head(struct object_id *oid, const char *action,
> if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
> BUG("Not a fully qualified branch: '%s'", switch_to_branch);
>
> - if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
> + if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
> ret = -1;
> goto leave_reset_head;
> }
Not touching the index, so no need to lock the index. Makes sense.
> @@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,
> if (!oid)
> oid = &head_oid;
>
> + if (flags & RESET_HEAD_REFS_ONLY)
> + goto reset_head_refs;
> +
Why not "refs_only" that we already prepared above??? Are we
munging that secondary variable before control comes here?
In any case, not touching the index nor the working tree, so no need
to call into the unpack_trees machinery. Makes sense.
> memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> setup_unpack_trees_porcelain(&unpack_tree_opts, action);
> unpack_tree_opts.head_idx = 1;
> @@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action,
> goto leave_reset_head;
> }
>
> +reset_head_refs:
> reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
And the control continues from the point we update the reflog.
Makes sense.
> strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
> prefix_len = msg.len;
This helper is touched by two other topics in flight, and that was
one of the reason why it took a bit longer than usual for me to
merge this topic. Please sanity-check the result of the conflict
resolution at the tip of 'pu' branch.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/4] rebase: avoid double reflog entry when switching branches
From: Junio C Hamano @ 2019-01-04 18:38 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <4c5f87b9dc245bf27785aa5559d4b35d87c4bcbf.1545398254.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When switching a branch *and* updating said branch to a different
> revision, let's avoid a double entry by first updating the branch and
> then adjusting the symbolic ref HEAD.
Ah, in the original sequence, HEAD is updated twice, leaving two
reflog entries for HEAD (and one for the underlying "switch_to"
branch by virtue of REF_UPDATE_VIA_HEAD). In the new sequence,
update_ref() updates the underlying "switch_to" and then HEAD, so
we'd get one reflog entry for each of them.
Makes sense. s/let's avoid a double entry/& in HEAD's reflog/ would
have avoided wasting reader's time who needlessly wondered where
that redundancy came from, though.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/rebase.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index e1dfa74ca8..768bea0da8 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -438,10 +438,11 @@ static int reset_head(struct object_id *oid, const char *action,
> detach_head ? REF_NO_DEREF : 0,
> UPDATE_REFS_MSG_ON_ERR);
> else {
> - ret = create_symref("HEAD", switch_to_branch, msg.buf);
> + ret = update_ref(reflog_orig_head, switch_to_branch, oid,
> + NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> if (!ret)
> - ret = update_ref(reflog_head, "HEAD", oid, NULL, 0,
> - UPDATE_REFS_MSG_ON_ERR);
> + ret = create_symref("HEAD", switch_to_branch,
> + reflog_head);
> }
>
> leave_reset_head:
^ permalink raw reply
* Re: [PATCH 1/4] rebase: move `reset_head()` into a better spot
From: Junio C Hamano @ 2019-01-04 18:39 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <175dc7d29a02d4ebf6b964f7821738f4ed2bbe0b.1545398254.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Over the next commits, we want to make use of it in `run_am()` (i.e.
> running the `--am` backend directly, without detouring to Unix shell
> script code) which in turn will be called from `run_specific_rebase()`.
>
> So let's move it before that latter function.
OK.
^ permalink raw reply
* Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree
From: Junio C Hamano @ 2019-01-04 18:40 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Johannes Schindelin, Orgad Shaneh,
Nguyễn Thái Ngọc Duy
In-Reply-To: <xmqqpntcclot.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> This is what the legacy (scripted) rebase does in
>> `move_to_original_branch`, and we will need this functionality in the
>> next commit.
>
> The move-to-original-branch helper does:
>
> - point $head_name to the commit pointed at by HEAD
> - point HEAD symref to $head_name
>
> without touching the index or the working tree files. It's not
> exactly "reset --soft" but more like "switch-branch --soft" ;-)
>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> builtin/rebase.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 768bea0da8..303175fdf1 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
>>
>> #define RESET_HEAD_DETACH (1<<0)
>> #define RESET_HEAD_HARD (1<<1)
>> +#define RESET_HEAD_REFS_ONLY (1<<2)
>
> In the future codebase in 'pu', we have 1<<2 already taken by
> another topic, so I'll tell my rerere database that the bit
> assignment needs to be adjusted.
>
>> static int reset_head(struct object_id *oid, const char *action,
>> const char *switch_to_branch, unsigned flags,
>> @@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action,
>> {
>> unsigned detach_head = flags & RESET_HEAD_DETACH;
>> unsigned reset_hard = flags & RESET_HEAD_HARD;
>> + unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
>> struct object_id head_oid;
>> struct tree_desc desc[2] = { { NULL }, { NULL } };
>> struct lock_file lock = LOCK_INIT;
>> @@ -359,7 +361,7 @@ static int reset_head(struct object_id *oid, const char *action,
>> if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
>> BUG("Not a fully qualified branch: '%s'", switch_to_branch);
>>
>> - if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
>> + if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
>> ret = -1;
>> goto leave_reset_head;
>> }
>
> Not touching the index, so no need to lock the index. Makes sense.
>
>> @@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,
>> if (!oid)
>> oid = &head_oid;
>>
>> + if (flags & RESET_HEAD_REFS_ONLY)
>> + goto reset_head_refs;
>> +
>
> Why not "refs_only" that we already prepared above??? Are we
> munging that secondary variable before control comes here?
>
> In any case, not touching the index nor the working tree, so no need
> to call into the unpack_trees machinery. Makes sense.
>
>> memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
>> setup_unpack_trees_porcelain(&unpack_tree_opts, action);
>> unpack_tree_opts.head_idx = 1;
>> @@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action,
>> goto leave_reset_head;
>> }
>>
>> +reset_head_refs:
>> reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
>
> And the control continues from the point we update the reflog.
> Makes sense.
>
>> strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
>> prefix_len = msg.len;
>
> This helper is touched by two other topics in flight, and that was
> one of the reason why it took a bit longer than usual for me to
> merge this topic. Please sanity-check the result of the conflict
> resolution at the tip of 'pu' branch.
The topics are os/rebase-runs-post-checkout-hook and nd/backup-log
IIRC.
^ permalink raw reply
* Re: [PATCH v2] test-lib: check Bash version for '-x' without using shell arrays
From: Carlo Arenas @ 2019-01-04 18:42 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Junio C Hamano, Johannes Sixt, Max Kirillov, Jeff King,
Eric Sunshine, git
In-Reply-To: <20190104123819.GD4673@szeder.dev>
v2 works fine, as expected
Carlo
^ permalink raw reply
* Re: [PATCH] diff: add support for reading files literally with --no-index
From: Junio C Hamano @ 2019-01-04 19:26 UTC (permalink / raw)
To: brian m. carlson; +Cc: Jonathan Nieder, git, Jeff King, Duy Nguyen
In-Reply-To: <20190104025724.GG423984@genre.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
>> - --dereference to control whether to follow symlinks
>
> This is actually surprisingly difficult. The reason I implemented this
> only for no-index mode is because there are actually several places we
> can stat a file in the diff code, and implementing a --dereference
> option that catches all of those cases and getting the option passed
> down to them is non-trivial.
Another thing to worry about is symlinks that point outside the
working tree. When a tracked content "dir/link" is a symlink to
"/etc/motd", it probably makes sense to open("/etc/motd") and read()
it on the working tree side of the diff, and probably even on the
index side of the diff, but what about obtaining contents for
"dir/link" in a year-old commit under --deference mode? I am not
sure if it makes sense to read from the filesystem in such a case.
I personally am perfectly fine if this "do not compare readlink(2),
but read contents literally" is limited to the --no-index mode.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] test-lib: check Bash version for '-x' without using shell arrays
From: Junio C Hamano @ 2019-01-04 20:04 UTC (permalink / raw)
To: Carlo Arenas
Cc: SZEDER Gábor, Johannes Sixt, Max Kirillov, Jeff King,
Eric Sunshine, git
In-Reply-To: <CAPUEspga6Kjv0pSYiD-BmZUCTf0rd=y0vneY_XvAkj_cCmXC8g@mail.gmail.com>
Carlo Arenas <carenas@gmail.com> writes:
> v2 works fine, as expected
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/2] Change how HTTP response body is returned
From: Junio C Hamano @ 2019-01-04 20:13 UTC (permalink / raw)
To: Jeff King; +Cc: Masaya Suzuki, git, jrnieder, sunshine
In-Reply-To: <20190104101149.GA26185@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The most robust thing would perhaps be:
>
> fflush(dest->file);
> ftruncate(fileno(dest->file), 0);
>
> which leaves the handle intact.
An added benefit of that approach is that there is no need for
the filename field in the dest structure.
Having a separate filename field could be a positive flexibility (it
could be used to open a file, store its FILE* in dest->file, while
storing the name of another file in dest->filename), but also a
negative flexibility (dest->file possibly pointing to a file
different from dest->filename is a source of confusion). As I do
not think any current caller that wants such a flexibility, or
callers in any immediate future, it probably is an overall plus if
we do not have to add the dest->filename field.
> (I agree with the rest of your review, especially that it would be
> easier to read if this were split into separate refactor and
> change-behavior steps).
>
> -Peff
^ permalink raw reply
* Re: [PATCH 1/1] Add optional targets for documentation l10n
From: Junio C Hamano @ 2019-01-04 21:05 UTC (permalink / raw)
To: Jean-Noël Avila; +Cc: git
In-Reply-To: <20190104165406.22358-2-jn.avila@free.fr>
Jean-Noël Avila <jn.avila@free.fr> writes:
> From: Jean-Noel Avila <jn.avila@free.fr>
>
> The standard doc lists can be filtered to allow using the compilation
> rules with translated manpages where all the pages of the original
> version may not be present.
>
> The install variable are reused in the secondary repo so that the
> configured paths can be used for translated manpages too.
>
> Signed-off-by: Jean-Noël Avila <jn.avila@free.fr>
> ---
> Documentation/Makefile | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index b5be2e2d3f..1f61a1fe86 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -35,13 +35,18 @@ MAN7_TXT += gittutorial-2.txt
> MAN7_TXT += gittutorial.txt
> MAN7_TXT += gitworkflows.txt
>
> -MAN_TXT = $(MAN1_TXT) $(MAN5_TXT) $(MAN7_TXT)
> +TMP_MAN_TXT = $(MAN1_TXT) $(MAN5_TXT) $(MAN7_TXT)
> +MAN_FILTER ?= $(TMP_MAN_TXT)
> +MAN_TXT = $(filter $(TMP_MAN_TXT), $(MAN_FILTER))
> +undefine TMP_MAN_TXT
> +
I think your arguments to $(filter) is the other way around, but
other than that, I think I get what you are trying to do. Let me
make sure I got it right.
The idea is to use $(filter PATTERN..., TEXT) that removes words in
TEXT that do not match any of the words in PATTERN, and for normal
build, MAN_FILTER is set identical to TMP_MAN_TXT (which is the
original MAN_TXT), so there is no filtering happen, but in a build
that does tweak MAN_FILTER, MAN_TXT can become a subset of the
original MAN_TXT.
Am I on the right track?
> MAN_XML = $(patsubst %.txt,%.xml,$(MAN_TXT))
> MAN_HTML = $(patsubst %.txt,%.html,$(MAN_TXT))
And these act on already-filtered MAN_TXT
> OBSOLETE_HTML += everyday.html
> OBSOLETE_HTML += git-remote-helpers.html
> -DOC_HTML = $(MAN_HTML) $(OBSOLETE_HTML)
> +
> +TMP_DOC_HTML = $(MAN_HTML) $(OBSOLETE_HTML)
>
> ARTICLES += howto-index
> ARTICLES += git-tools
> @@ -81,11 +86,14 @@ TECH_DOCS += technical/trivial-merge
> SP_ARTICLES += $(TECH_DOCS)
> SP_ARTICLES += technical/api-index
>
> -DOC_HTML += $(patsubst %,%.html,$(ARTICLES) $(SP_ARTICLES))
> +TMP_DOC_HTML += $(patsubst %,%.html,$(ARTICLES) $(SP_ARTICLES))
> +HTML_FILTER ?= $(TMP_DOC_HTML)
> +DOC_HTML = $(filter $(HTML_FILTER),$(TMP_DOC_HTML))
> +undefine TMP_DOC_HTML
This one uses $(filter) in the right direction.
So is it expected that HTML help pages that correspond to manpages
are strict subset of manpages?
I see HTML_FILTER may be useful to filter HTML pages that come from
$(ARTICLES), but I'd expect that all $(MAN_HTML) that came from the
already-filtered $(MAN_TXT) would not require any further filtering.
With the approach shown, the secondary project ends up needing to
list all the translated MAN_TXT twice (once for MAN_FILTER, and
again for HTML_FILTER), doesn't it?
I am wondering if it makes more sense to have HTML_FILTER filter _only_
parts of the DOC_HTML that does not come from MAN_TXT (i.e. those
$(ARTICLES) pages).
> -DOC_MAN1 = $(patsubst %.txt,%.1,$(MAN1_TXT))
> -DOC_MAN5 = $(patsubst %.txt,%.5,$(MAN5_TXT))
> -DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
> +DOC_MAN1 = $(patsubst %.txt,%.1,$(filter $(MAN_FILTER), $(MAN1_TXT)))
> +DOC_MAN5 = $(patsubst %.txt,%.5,$(filter $(MAN_FILTER), $(MAN5_TXT)))
> +DOC_MAN7 = $(patsubst %.txt,%.7,$(filter $(MAN_FILTER), $(MAN7_TXT)))
These are OK, too.
By the way, lose the SP after ',' in $(filter). As we can see in
the context lines in the patch, args to $(make-functions) are
separated with comma without surrounding SP by convention.
What kind of PATTERN does the secondary project supply when invoking
this Makefile? If it is list of filenames, I am wondering if it is
simpler to have it override MAN{1,5,7}_TXT variables, without adding
these "TMP_* + fliter + undef TMP_*" dance.
> prefix ?= $(HOME)
> bindir ?= $(prefix)/bin
> @@ -444,4 +452,9 @@ print-man1:
> lint-docs::
> $(QUIET_LINT)$(PERL_PATH) lint-gitlink.perl
>
> +ifeq ($(wildcard po/Makefile),po/Makefile)
> +doc-l10n install-l10n::
> + $(MAKE) -C po $@
> +endif
> +
> .PHONY: FORCE
^ permalink raw reply
* Re: [PATCH v2] worktree: allow to (re)move worktrees with uninitialized submodules
From: Junio C Hamano @ 2019-01-04 22:51 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: sunshine, git
In-Reply-To: <20181216144657.31181-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Uninitialized submodules have nothing valueable for us to be worried
> about. They are just SHA-1. Let "worktree remove" and "worktree move"
> continue in this case so that people can still use multiple worktrees
> on repos with optional submodules that are never populated, like
> sha1collisiondetection in git.git when checked out by doc-diff script.
>
> Note that for "worktree remove", it is possible that a user
> initializes a submodule (*), makes some commits (but not push), then
> deinitializes it. At that point, the submodule is unpopulated, but the
> precious new commits are still in
>
> $GIT_COMMON_DIR/worktrees/<worktree>/modules/<submodule>
>
> directory and we should not allow removing the worktree or we lose
> those commits forever. The new directory check is added to prevent
> this.
>
> (*) yes they are screwed anyway by doing this since "git submodule"
> would add submodule.* in $GIT_COMMON_DIR/config, which is shared
> across multiple worktrees. But it does not mean we let them be
> screwed even more.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> Fixed Eric's comment. I was a bit annoyed by the duplicate die() too
> but didn't think of adding "else" in front of "if (read_index"
>
> builtin/worktree.c | 29 +++++++++++++++++++++++------
> 1 file changed, 23 insertions(+), 6 deletions(-)
Is this a fair description for this 1-patch topic?
"git worktree remove" and "git worktree move" failed to work
when there is an uninitialized submodule, which has been fixed.
If so, can we have a test case to cover this fix?
Thanks.
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 5e84026177..3f9907fcc9 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -9,6 +9,7 @@
> #include "refs.h"
> #include "run-command.h"
> #include "sigchain.h"
> +#include "submodule.h"
> #include "refs.h"
> #include "utf8.h"
> #include "worktree.h"
> @@ -724,20 +725,36 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
> static void validate_no_submodules(const struct worktree *wt)
> {
> struct index_state istate = { NULL };
> + struct strbuf path = STRBUF_INIT;
> int i, found_submodules = 0;
>
> - if (read_index_from(&istate, worktree_git_path(wt, "index"),
> - get_worktree_git_dir(wt)) > 0) {
> + if (is_directory(worktree_git_path(wt, "modules"))) {
> + /*
> + * There could be false positives, e.g. the "modules"
> + * directory exists but is empty. But it's a rare case and
> + * this simpler check is probably good enough for now.
> + */
> + found_submodules = 1;
> + } else if (read_index_from(&istate, worktree_git_path(wt, "index"),
> + get_worktree_git_dir(wt)) > 0) {
> for (i = 0; i < istate.cache_nr; i++) {
> struct cache_entry *ce = istate.cache[i];
> + int err;
>
> - if (S_ISGITLINK(ce->ce_mode)) {
> - found_submodules = 1;
> - break;
> - }
> + if (!S_ISGITLINK(ce->ce_mode))
> + continue;
> +
> + strbuf_reset(&path);
> + strbuf_addf(&path, "%s/%s", wt->path, ce->name);
> + if (!is_submodule_populated_gently(path.buf, &err))
> + continue;
> +
> + found_submodules = 1;
> + break;
> }
> }
> discard_index(&istate);
> + strbuf_release(&path);
>
> if (found_submodules)
> die(_("working trees containing submodules cannot be moved or removed"));
^ permalink raw reply
* Re: [PATCH 4/4] built-in rebase: call `git am` directly
From: Junio C Hamano @ 2019-01-04 23:19 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <2b5ece8263936f0a7dfad864c0de43d784fdaf1f.1545398254.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> While the scripted `git rebase` still has to rely on the
> `git-rebase--am.sh` script to implement the glue between the `rebase`
> and the `am` commands, we can go a more direct route in the built-in
> rebase and avoid using a shell script altogether.
...
> builtin/rebase.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
Now at some point as a follow-up change, we'd need to remove the
git-rebase--am.sh that is no longer used, together with the
reference to it in the Makefile, no?
^ permalink raw reply
* [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
In-Reply-To: <20181230191629.3232-1-szeder.dev@gmail.com>
To recap: this patch series tries to make reproducing rare failures in
flaky tests easier: it adds the '--stress' option to our test library
to run the test script repeatedly in multiple parallel jobs, in the
hope that the increased load creates enough variance in the timing of
the test's commands that such a failure is eventually triggered.
There is an issue with v3: by parsing all command line options before
re-executing the test script for '--tee' or '--verbose-log', we not
only parse the options twice (which is harmless), but check the
shell's version for untraceable test scripts twice as well.
Consequently, the warning about ignoring '-x' are issued twice:
$ ./t1510-repo-setup.sh -x --tee
warning: ignoring -x; './t1510-repo-setup.sh' is untraceable without BASH_XTRACEFD
warning: ignoring -x; './t1510-repo-setup.sh' is untraceable without BASH_XTRACEFD
ok 1 - #0: nonbare repo, no explicit configuration
<...>
Furthermore, when TEST_SHELL_PATH is not /bin/sh but is set to Bash
v4.1 or later, then the above command issues one warning as it's run
with /bin/sh from the shebang line, but then re-execs itself with Bash
and does print the '-x' tracing output as well.
This version of the patch series fixes the above issue by checking the
shell's version after '--tee' and '--verbose-log' re-execue the test
script. To do so it makes more sense to move the 'test-lib: extract
Bash version check for '-x' tracing' patch to the beginning of the
series. This move, however, made the range-diff quite noisy, hence
the interdiff below, as it describes the changes since v3 much better.
It's based on 'sg/test-bash-version-fix'.
SZEDER Gábor (8):
test-lib: translate SIGTERM and SIGHUP to an exit
test-lib: extract Bash version check for '-x' tracing
test-lib: parse options in a for loop to keep $@ intact
test-lib: parse command line options earlier
test-lib: consolidate naming of test-results paths
test-lib: set $TRASH_DIRECTORY earlier
test-lib-functions: introduce the 'test_set_port' helper function
test-lib: add the '--stress' option to run a test repeatedly under
load
t/README | 19 +-
t/lib-git-daemon.sh | 2 +-
t/lib-git-p4.sh | 9 +-
t/lib-git-svn.sh | 2 +-
t/lib-httpd.sh | 2 +-
t/t0410-partial-clone.sh | 1 -
t/t5512-ls-remote.sh | 2 +-
t/test-lib-functions.sh | 39 +++++
t/test-lib.sh | 365 ++++++++++++++++++++++++++-------------
9 files changed, 308 insertions(+), 133 deletions(-)
Interdiff:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3f0b0c07df..a1abb1177a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -180,29 +180,6 @@ then
immediate=t
fi
-if test -n "$trace" && test -n "$test_untraceable"
-then
- # '-x' tracing requested, but this test script can't be reliably
- # traced, unless it is run with a Bash version supporting
- # BASH_XTRACEFD (introduced in Bash v4.1).
- if test -n "$BASH_VERSION" && eval '
- test ${BASH_VERSINFO[0]} -gt 4 || {
- test ${BASH_VERSINFO[0]} -eq 4 &&
- test ${BASH_VERSINFO[1]} -ge 1
- }
- '
- then
- : Executed by a Bash version supporting BASH_XTRACEFD. Good.
- else
- echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
- trace=
- fi
-fi
-if test -n "$trace" && test -z "$verbose_log"
-then
- verbose=t
-fi
-
TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}"
TEST_NAME="$(basename "$0" .sh)"
TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
@@ -322,6 +299,34 @@ then
exit
fi
+if test -n "$trace" && test -n "$test_untraceable"
+then
+ # '-x' tracing requested, but this test script can't be reliably
+ # traced, unless it is run with a Bash version supporting
+ # BASH_XTRACEFD (introduced in Bash v4.1).
+ #
+ # Perform this version check _after_ the test script was
+ # potentially re-executed with $TEST_SHELL_PATH for '--tee' or
+ # '--verbose-log', so the right shell is checked and the
+ # warning is issued only once.
+ if test -n "$BASH_VERSION" && eval '
+ test ${BASH_VERSINFO[0]} -gt 4 || {
+ test ${BASH_VERSINFO[0]} -eq 4 &&
+ test ${BASH_VERSINFO[1]} -ge 1
+ }
+ '
+ then
+ : Executed by a Bash version supporting BASH_XTRACEFD. Good.
+ else
+ echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
+ trace=
+ fi
+fi
+if test -n "$trace" && test -z "$verbose_log"
+then
+ verbose=t
+fi
+
# For repeatability, reset the environment to known value.
# TERM is sanitized below, after saving color control sequences.
LANG=C
--
2.20.1.151.gec613c4b75
^ permalink raw reply related
* [PATCH v4 1/8] test-lib: translate SIGTERM and SIGHUP to an exit
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
In-Reply-To: <20190105010859.11031-1-szeder.dev@gmail.com>
Right now if a test script receives SIGTERM or SIGHUP (e.g., because a
test was hanging and the user 'kill'-ed it or simply closed the
terminal window the test was running in), the shell exits immediately.
This can be annoying if the test script did any global setup, like
starting apache or git-daemon, as it will not have an opportunity to
clean up after itself. A subsequent run of the test won't be able to
start its own daemon, and will either fail or skip the tests.
Instead, let's trap SIGTERM and SIGHUP as well to make sure we do a
clean shutdown, and just chain it to a normal exit (which will trigger
any cleanup).
This patch follows suit of da706545f7 (t: translate SIGINT to an exit,
2015-03-13), and even stole its commit message as well.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c34831a4de..4c3744cce4 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -476,7 +476,7 @@ die () {
GIT_EXIT_OK=
trap 'die' EXIT
-trap 'exit $?' INT
+trap 'exit $?' INT TERM HUP
# The user-facing functions are loaded from a separate file so that
# test_perf subshells can have them too
--
2.20.1.151.gec613c4b75
^ permalink raw reply related
* [PATCH v4 2/8] test-lib: extract Bash version check for '-x' tracing
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
In-Reply-To: <20190105010859.11031-1-szeder.dev@gmail.com>
One of our test scripts, 't1510-repo-setup.sh' [1], still can't be
reliably run with '-x' tracing enabled, unless it's executed with a
Bash version supporting BASH_XTRACEFD (since v4.1). We have a lengthy
condition to check the version of the shell running the test script,
and disable tracing if it's not executed with a suitable Bash version
[2].
Move this check out from the option parsing loop, so other options can
imply '-x' by setting 'trace=t', without missing this Bash version
check.
[1] 5827506928 (t1510-repo-setup: mark as untraceable with '-x',
2018-02-24)
[2] 5fc98e79fc (t: add means to disable '-x' tracing for individual
test scripts, 2018-02-24)
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4c3744cce4..1f02e2e25b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -317,24 +317,7 @@ do
GIT_TEST_CHAIN_LINT=0
shift ;;
-x)
- # Some test scripts can't be reliably traced with '-x',
- # unless the test is run with a Bash version supporting
- # BASH_XTRACEFD (introduced in Bash v4.1). Check whether
- # this test is marked as such, and ignore '-x' if it
- # isn't executed with a suitable Bash version.
- if test -z "$test_untraceable" || {
- test -n "$BASH_VERSION" && eval '
- test ${BASH_VERSINFO[0]} -gt 4 || {
- test ${BASH_VERSINFO[0]} -eq 4 &&
- test ${BASH_VERSINFO[1]} -ge 1
- }
- '
- }
- then
- trace=t
- else
- echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
- fi
+ trace=t
shift ;;
-V|--verbose-log)
verbose_log=t
@@ -353,6 +336,24 @@ then
test -z "$verbose_log" && verbose=t
fi
+if test -n "$trace" && test -n "$test_untraceable"
+then
+ # '-x' tracing requested, but this test script can't be reliably
+ # traced, unless it is run with a Bash version supporting
+ # BASH_XTRACEFD (introduced in Bash v4.1).
+ if test -n "$BASH_VERSION" && eval '
+ test ${BASH_VERSINFO[0]} -gt 4 || {
+ test ${BASH_VERSINFO[0]} -eq 4 &&
+ test ${BASH_VERSINFO[1]} -ge 1
+ }
+ '
+ then
+ : Executed by a Bash version supporting BASH_XTRACEFD. Good.
+ else
+ echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
+ trace=
+ fi
+fi
if test -n "$trace" && test -z "$verbose_log"
then
verbose=t
--
2.20.1.151.gec613c4b75
^ permalink raw reply related
* [PATCH v4 3/8] test-lib: parse options in a for loop to keep $@ intact
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
In-Reply-To: <20190105010859.11031-1-szeder.dev@gmail.com>
'test-lib.sh' looks for the presence of certain options like '--tee'
and '--verbose-log', so it can execute the test script again to save
its standard output and error, and to do so it needs the original
command line options the test was invoked with.
The next patch is about to move the option parsing loop earlier in
'test-lib.sh', but it is implemented using 'shift' in a while loop,
effecively destroying "$@" by the end of the option parsing. Not
good.
As a preparatory step, turn that option parsing loop into a 'for opt
in "$@"' loop to preserve "$@" intact while iterating over the
options, and taking extra care to handle the '-r' option's required
argument (or the lack thereof).
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 78 +++++++++++++++++++++++++++------------------------
1 file changed, 42 insertions(+), 36 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1f02e2e25b..3cf59a92f0 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -264,68 +264,74 @@ test "x$TERM" != "xdumb" && (
) &&
color=t
-while test "$#" -ne 0
+store_arg_to=
+prev_opt=
+for opt
do
- case "$1" in
+ if test -n "$store_arg_to"
+ then
+ eval $store_arg_to=\$opt
+ store_arg_to=
+ prev_opt=
+ continue
+ fi
+
+ case "$opt" in
-d|--d|--de|--deb|--debu|--debug)
- debug=t; shift ;;
+ debug=t ;;
-i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
- immediate=t; shift ;;
+ immediate=t ;;
-l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests)
- GIT_TEST_LONG=t; export GIT_TEST_LONG; shift ;;
+ GIT_TEST_LONG=t; export GIT_TEST_LONG ;;
-r)
- shift; test "$#" -ne 0 || {
- echo 'error: -r requires an argument' >&2;
- exit 1;
- }
- run_list=$1; shift ;;
+ store_arg_to=run_list
+ ;;
--run=*)
- run_list=${1#--*=}; shift ;;
+ run_list=${opt#--*=} ;;
-h|--h|--he|--hel|--help)
- help=t; shift ;;
+ help=t ;;
-v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
- verbose=t; shift ;;
+ verbose=t ;;
--verbose-only=*)
- verbose_only=${1#--*=}
- shift ;;
+ verbose_only=${opt#--*=}
+ ;;
-q|--q|--qu|--qui|--quie|--quiet)
# Ignore --quiet under a TAP::Harness. Saying how many tests
# passed without the ok/not ok details is always an error.
- test -z "$HARNESS_ACTIVE" && quiet=t; shift ;;
+ test -z "$HARNESS_ACTIVE" && quiet=t ;;
--with-dashes)
- with_dashes=t; shift ;;
+ with_dashes=t ;;
--no-color)
- color=; shift ;;
+ color= ;;
--va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
- valgrind=memcheck
- shift ;;
+ valgrind=memcheck ;;
--valgrind=*)
- valgrind=${1#--*=}
- shift ;;
+ valgrind=${opt#--*=} ;;
--valgrind-only=*)
- valgrind_only=${1#--*=}
- shift ;;
+ valgrind_only=${opt#--*=} ;;
--tee)
- shift ;; # was handled already
+ ;; # was handled already
--root=*)
- root=${1#--*=}
- shift ;;
+ root=${opt#--*=} ;;
--chain-lint)
- GIT_TEST_CHAIN_LINT=1
- shift ;;
+ GIT_TEST_CHAIN_LINT=1 ;;
--no-chain-lint)
- GIT_TEST_CHAIN_LINT=0
- shift ;;
+ GIT_TEST_CHAIN_LINT=0 ;;
-x)
- trace=t
- shift ;;
+ trace=t ;;
-V|--verbose-log)
- verbose_log=t
- shift ;;
+ verbose_log=t ;;
*)
- echo "error: unknown test option '$1'" >&2; exit 1 ;;
+ echo "error: unknown test option '$opt'" >&2; exit 1 ;;
esac
+
+ prev_opt=$opt
done
+if test -n "$store_arg_to"
+then
+ echo "error: $prev_opt requires an argument" >&2
+ exit 1
+fi
if test -n "$valgrind_only"
then
--
2.20.1.151.gec613c4b75
^ permalink raw reply related
* [PATCH v4 4/8] test-lib: parse command line options earlier
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
In-Reply-To: <20190105010859.11031-1-szeder.dev@gmail.com>
'test-lib.sh' looks for the presence of certain options like '--tee'
and '--verbose-log', so it can execute the test script again to save
its standard output and error. It looks for '--valgrind' as well, to
set up some Valgrind-specific stuff. These all happen before the
actual option parsing loop, and the conditions looking for these
options look a bit odd, too. They are not completely correct, either,
because in a bogus invocation like './t1234-foo.sh -r --tee' they
recognize '--tee', although it should be handled as the required
argument of the '-r' option. This patch series will add two more
options to look out for early, and, in addition, will have to extract
these options' stuck arguments (i.e. '--opt=arg') as well.
So let's move the option parsing loop and the couple of related
conditions following it earlier in 'test-lib.sh', before the place
where the test script is executed again for '--tee' and its friends.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 233 +++++++++++++++++++++++++++-----------------------
1 file changed, 124 insertions(+), 109 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3cf59a92f0..14ccb60838 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -71,13 +71,102 @@ then
exit 1
fi
+# Parse options while taking care to leave $@ intact, so we will still
+# have all the original command line options when executing the test
+# script again for '--tee' and '--verbose-log' below.
+store_arg_to=
+prev_opt=
+for opt
+do
+ if test -n "$store_arg_to"
+ then
+ eval $store_arg_to=\$opt
+ store_arg_to=
+ prev_opt=
+ continue
+ fi
+
+ case "$opt" in
+ -d|--d|--de|--deb|--debu|--debug)
+ debug=t ;;
+ -i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
+ immediate=t ;;
+ -l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests)
+ GIT_TEST_LONG=t; export GIT_TEST_LONG ;;
+ -r)
+ store_arg_to=run_list
+ ;;
+ --run=*)
+ run_list=${opt#--*=} ;;
+ -h|--h|--he|--hel|--help)
+ help=t ;;
+ -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
+ verbose=t ;;
+ --verbose-only=*)
+ verbose_only=${opt#--*=}
+ ;;
+ -q|--q|--qu|--qui|--quie|--quiet)
+ # Ignore --quiet under a TAP::Harness. Saying how many tests
+ # passed without the ok/not ok details is always an error.
+ test -z "$HARNESS_ACTIVE" && quiet=t ;;
+ --with-dashes)
+ with_dashes=t ;;
+ --no-color)
+ color= ;;
+ --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
+ valgrind=memcheck
+ tee=t
+ ;;
+ --valgrind=*)
+ valgrind=${opt#--*=}
+ tee=t
+ ;;
+ --valgrind-only=*)
+ valgrind_only=${opt#--*=}
+ tee=t
+ ;;
+ --tee)
+ tee=t ;;
+ --root=*)
+ root=${opt#--*=} ;;
+ --chain-lint)
+ GIT_TEST_CHAIN_LINT=1 ;;
+ --no-chain-lint)
+ GIT_TEST_CHAIN_LINT=0 ;;
+ -x)
+ trace=t ;;
+ -V|--verbose-log)
+ verbose_log=t
+ tee=t
+ ;;
+ *)
+ echo "error: unknown test option '$opt'" >&2; exit 1 ;;
+ esac
+
+ prev_opt=$opt
+done
+if test -n "$store_arg_to"
+then
+ echo "error: $prev_opt requires an argument" >&2
+ exit 1
+fi
+
+if test -n "$valgrind_only"
+then
+ test -z "$valgrind" && valgrind=memcheck
+ test -z "$verbose" && verbose_only="$valgrind_only"
+elif test -n "$valgrind"
+then
+ test -z "$verbose_log" && verbose=t
+fi
+
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
-case "$GIT_TEST_TEE_STARTED, $* " in
-done,*)
- # do not redirect again
- ;;
-*' --tee '*|*' --va'*|*' -V '*|*' --verbose-log '*)
+if test "$GIT_TEST_TEE_STARTED" = "done"
+then
+ : # do not redirect again
+elif test -n "$tee"
+then
mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
@@ -94,8 +183,35 @@ done,*)
echo $? >"$BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
test "$(cat "$BASE.exit")" = 0
exit
- ;;
-esac
+fi
+
+if test -n "$trace" && test -n "$test_untraceable"
+then
+ # '-x' tracing requested, but this test script can't be reliably
+ # traced, unless it is run with a Bash version supporting
+ # BASH_XTRACEFD (introduced in Bash v4.1).
+ #
+ # Perform this version check _after_ the test script was
+ # potentially re-executed with $TEST_SHELL_PATH for '--tee' or
+ # '--verbose-log', so the right shell is checked and the
+ # warning is issued only once.
+ if test -n "$BASH_VERSION" && eval '
+ test ${BASH_VERSINFO[0]} -gt 4 || {
+ test ${BASH_VERSINFO[0]} -eq 4 &&
+ test ${BASH_VERSINFO[1]} -ge 1
+ }
+ '
+ then
+ : Executed by a Bash version supporting BASH_XTRACEFD. Good.
+ else
+ echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
+ trace=
+ fi
+fi
+if test -n "$trace" && test -z "$verbose_log"
+then
+ verbose=t
+fi
# For repeatability, reset the environment to known value.
# TERM is sanitized below, after saving color control sequences.
@@ -193,7 +309,7 @@ fi
# Add libc MALLOC and MALLOC_PERTURB test
# only if we are not executing the test with valgrind
-if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
+if test -n "$valgrind" ||
test -n "$TEST_NO_MALLOC_CHECK"
then
setup_malloc_check () {
@@ -264,107 +380,6 @@ test "x$TERM" != "xdumb" && (
) &&
color=t
-store_arg_to=
-prev_opt=
-for opt
-do
- if test -n "$store_arg_to"
- then
- eval $store_arg_to=\$opt
- store_arg_to=
- prev_opt=
- continue
- fi
-
- case "$opt" in
- -d|--d|--de|--deb|--debu|--debug)
- debug=t ;;
- -i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
- immediate=t ;;
- -l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests)
- GIT_TEST_LONG=t; export GIT_TEST_LONG ;;
- -r)
- store_arg_to=run_list
- ;;
- --run=*)
- run_list=${opt#--*=} ;;
- -h|--h|--he|--hel|--help)
- help=t ;;
- -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
- verbose=t ;;
- --verbose-only=*)
- verbose_only=${opt#--*=}
- ;;
- -q|--q|--qu|--qui|--quie|--quiet)
- # Ignore --quiet under a TAP::Harness. Saying how many tests
- # passed without the ok/not ok details is always an error.
- test -z "$HARNESS_ACTIVE" && quiet=t ;;
- --with-dashes)
- with_dashes=t ;;
- --no-color)
- color= ;;
- --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
- valgrind=memcheck ;;
- --valgrind=*)
- valgrind=${opt#--*=} ;;
- --valgrind-only=*)
- valgrind_only=${opt#--*=} ;;
- --tee)
- ;; # was handled already
- --root=*)
- root=${opt#--*=} ;;
- --chain-lint)
- GIT_TEST_CHAIN_LINT=1 ;;
- --no-chain-lint)
- GIT_TEST_CHAIN_LINT=0 ;;
- -x)
- trace=t ;;
- -V|--verbose-log)
- verbose_log=t ;;
- *)
- echo "error: unknown test option '$opt'" >&2; exit 1 ;;
- esac
-
- prev_opt=$opt
-done
-if test -n "$store_arg_to"
-then
- echo "error: $prev_opt requires an argument" >&2
- exit 1
-fi
-
-if test -n "$valgrind_only"
-then
- test -z "$valgrind" && valgrind=memcheck
- test -z "$verbose" && verbose_only="$valgrind_only"
-elif test -n "$valgrind"
-then
- test -z "$verbose_log" && verbose=t
-fi
-
-if test -n "$trace" && test -n "$test_untraceable"
-then
- # '-x' tracing requested, but this test script can't be reliably
- # traced, unless it is run with a Bash version supporting
- # BASH_XTRACEFD (introduced in Bash v4.1).
- if test -n "$BASH_VERSION" && eval '
- test ${BASH_VERSINFO[0]} -gt 4 || {
- test ${BASH_VERSINFO[0]} -eq 4 &&
- test ${BASH_VERSINFO[1]} -ge 1
- }
- '
- then
- : Executed by a Bash version supporting BASH_XTRACEFD. Good.
- else
- echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
- trace=
- fi
-fi
-if test -n "$trace" && test -z "$verbose_log"
-then
- verbose=t
-fi
-
if test -n "$color"
then
# Save the color control sequences now rather than run tput
--
2.20.1.151.gec613c4b75
^ permalink raw reply related
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