* Re: [PATCH] Always check the return value of `repo_read_object_file()`
From: Johannes Schindelin @ 2024-02-12 23:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <xmqqplx9fdyk.fsf@gitster.g>
Hi Junio,
On Tue, 6 Feb 2024, Junio C Hamano wrote:
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index c8e33f97755..982bcfc4b1d 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -571,6 +571,8 @@ static int grep_cache(struct grep_opt *opt,
> >
> > data = repo_read_object_file(the_repository, &ce->oid,
> > &type, &size);
> > + if (!data)
> > + die(_("unable to read tree %s"), oid_to_hex(&ce->oid));
> > init_tree_desc(&tree, data, size);
> >
> > hit |= grep_tree(opt, pathspec, &tree, &name, 0, 0);
>
> This caught my attention during today's integration cycle. Checking
> nullness for data certainly is an improvement, but shouldn't we be
> checking type as well to make sure it is a tree and not a random
> tree-ish or even blob?
That sounds right, but I am already stretching this patch beyond what I am
funded to work on, and therefore I need to leave it at the current state.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v4 00/28] Enrich Trailer API
From: Christian Couder @ 2024-02-12 23:37 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <pull.1632.v4.git.1707196348.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> This patch series is the first 10 patches of a larger cleanup/bugfix series
> (henceforth "larger series") I've been working on.
There are now 28 patches in this series.
I took a look at all of them, and I think that this series should be
split into 4 or more series.
For example perhaps one series until the "trailer: move
interpret_trailers() to interpret-trailers.c" patch, then another one
until "trailer: finish formatting unification". etc.
Also I think it might be possible to avoid some test failures
introduced by some patches. If it's not possible, I agree with Junio
that it would be nice if the failing tests were changed to use
'test_expect_failure'.
Also it seems to me that many patches towards the end of this series
should be split.
> The main goal of this
> series is to begin the process of "libifying" the trailer API. By "API" I
> mean the interface exposed in trailer.h. The larger series brings a number
> of additional cleanups (exposing and fixing some bugs along the way), and
> builds on top of this series.
[...]
> With the libification-focused goals out of the way, let's turn to this patch
> series in more detail.
I like the goal of libifying Git the trailer API, and the way you want
to do it seems reasonable to me.
[...]
> In summary this series breaks up "process_trailers()" into smaller pieces,
> exposing many of the parts relevant to trailer-related processing in
> trailer.h. This will force us to eventually introduce unit tests for these
> API functions, but that is a good thing for API stability.
I am a bit sad that this series doesn't introduce unit tests using the
new test framework in C yet. I understand that this series is mostly a
big refactoring and maybe it's better to introduce unit tests only
when the refactoring is finished though. Anyway I hope the next series
will introduce such tests.
> In the future after libification is "complete", users external to Git will
> be able to use the same trailer processing API used by the
> interpret-trailers builtin. For example, a web server may want to parse
> trailers the same way that Git would parse them, without having to call
> interpret-trailers as a subprocess. This use case was the original
> motivation behind my work in this area.
Thanks for telling us about this use case.
> Thanks to the aggressive refactoring in this series, I've been able to
> identify and fix several bugs in our existing implementation. Those fixes
> build on top of this series but were not included here, in order to keep
> this series small. Below is a "shortlog" of those fixes I have locally:
>
> * "trailer: trailer replacement should not change its position" (If we
> found a trailer we'd like to replace, preserve its position relative to
> the other trailers found in the trailer block, instead of always moving
> it to the beginning or end of the entire trailer block.)
I believe there was a reason why it was done this way. I don't
remember what it was though.
> * "interpret-trailers: preserve trailers coming from the input" (Sometimes,
> the parsed trailers from the input will be formatted differently
> depending on whether we provide --only-trailers or not. Make the trailers
> that were not modified and which are coming directly from the input get
> formatted the same way, regardless of this flag.)
It could be a feature to be able to normalize trailers in a certain way.
> * "interpret-trailers: do not modify the input if NOP" (Refrain from
> subtracting or adding a newline around the patch divider "---" if we are
> not adding new trailers.)
It could be a feature to be able to normalize this too.
> * "trailer formatter: split up format_trailer() monolith" (Fix a bug in
> git-log where we still printed a blank newline even if we didn't want to
> format anything.)
I am not sure this is a bug fix either. It could perhaps be a normalization too.
> * "interpret-trailers: fail if given unrecognized arguments" (E.g., for
> "--where", only accept recognized WHERE_* enum values. If we get
> something unrecognized, fail with an error instead of silently doing
> nothing. Ditto for "--if-exists" and "--if-missing".)
It's possible that there was a reason why it was done this way.
I think you might want to take a look at the discussions on the
mailing list when "interpret-trailers" was developed. There were a lot
of discussions over a long time, and there were a lot of requests and
suggestions about what it should do.
^ permalink raw reply
* Re: [PATCH v4 12/28] format_trailer_info(): append newline for non-trailer lines
From: Christian Couder @ 2024-02-12 23:37 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <a72eca301f7f9016ef3a8063f79790ce00f41ffe.1707196348.git.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
[...]
> The test suite passes again, so format_trailer_info() is in better shape
> supersede format_trailers(), which we'll do in the next patch.
s/supersede/to supersede/
^ permalink raw reply
* Re: [PATCH v4 15/28] format_trailer_info(): avoid double-printing the separator
From: Christian Couder @ 2024-02-12 23:38 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <ba1f387747b08a7270f7387beddd75dc4a8eddfe.1707196348.git.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Linus Arver <linusa@google.com>
>
> Do not hardcode the printing of ": " as the separator and space (which
> can result in double-printing these characters); instead only
> print the separator and space if we cannot find any recognized separator
> somewhere in the key (yes, keys may have a trailing separator in it ---
> we will eventually fix this design but not now). Do so by copying the
> code out of print_tok_val(), and deleting the same function.
>
> The test suite passes again with this change.
I think it should be clearer above that this fixes a bug that was
introduced earlier in the series.
Also I wonder why it was not possible to modify format_trailer_info()
like it is done in this patch before using it to replace
format_trailers().
^ permalink raw reply
* Re: [PATCH v4 14/28] format_trailer_info(): teach it about opts->trim_empty
From: Christian Couder @ 2024-02-12 23:38 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <11f854399db2b0da5d82cad910c3b86ca9c2e0db.1707196348.git.gitgitgadget@gmail.com>
> diff --git a/trailer.c b/trailer.c
> index f4defad3dae..c28b6c11cc5 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -162,20 +162,6 @@ static void print_tok_val(struct strbuf *out, const char *tok, const char *val)
> strbuf_addf(out, "%s%c %s\n", tok, separators[0], val);
> }
>
> -static void format_trailers(const struct process_trailer_options *opts,
> - struct list_head *trailers,
> - struct strbuf *out)
> -{
> - struct list_head *pos;
> - struct trailer_item *item;
> - list_for_each(pos, trailers) {
> - item = list_entry(pos, struct trailer_item, list);
> - if ((!opts->trim_empty || strlen(item->value) > 0) &&
> - (!opts->only_trailers || item->token))
> - print_tok_val(out, item->token, item->value);
> - }
> -}
It seems to me that this function could and should have been removed
in the previous patch. If there is a reason why it is better to do it
in this patch, I think it should be explained more clearly in the
commit message.
> static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
> {
> struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
> @@ -1101,6 +1087,15 @@ void format_trailer_info(const struct process_trailer_options *opts,
> strbuf_addstr(&tok, item->token);
> strbuf_addstr(&val, item->value);
>
> + /*
> + * Skip key/value pairs where the value was empty. This
> + * can happen from trailers specified without a
> + * separator, like `--trailer "Reviewed-by"` (no
> + * corresponding value).
> + */
> + if (opts->trim_empty && !strlen(item->value))
> + continue;
> +
Wasn't it possible to make this change in format_trailer_info() before
using format_trailer_info() to replace format_trailers()?
> if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
> if (opts->separator && out->len != origlen)
> strbuf_addbuf(out, opts->separator);
^ permalink raw reply
* Re: [PATCH v4 16/28] trailer: finish formatting unification
From: Christian Couder @ 2024-02-12 23:38 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <31725832224e3d6b14066af8a87eaf4ab589179e.1707196348.git.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> /*
> - * Format the trailers from the commit msg "msg" into the strbuf "out".
> - * Note two caveats about "opts":
> - *
> - * - this is primarily a helper for pretty.c, and not
> - * all of the flags are supported.
> - *
> - * - this differs from process_trailers slightly in that we always format
> - * only the trailer block itself, even if the "only_trailers" option is not
> - * set.
This makes me wonder if there was actually a good reason why
format_trailers() and format_trailer_info() were 2 different
functions. Is there info about this in the commit message of the
commit which introduced this comment?
^ permalink raw reply
* Re: [PATCH v4 19/28] trailer: make trailer_info struct private
From: Christian Couder @ 2024-02-12 23:38 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <f5f0d06613f2701af9bd3a1a9274aae232e03d8f.1707196348.git.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 11f4ce9e4a2..6bf8cec005a 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -141,7 +141,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
> LIST_HEAD(head);
> struct strbuf sb = STRBUF_INIT;
> struct strbuf trailer_block = STRBUF_INIT;
> - struct trailer_info info;
> + struct trailer_info *info;
> FILE *outfile = stdout;
>
> trailer_config_init();
> @@ -151,13 +151,13 @@ static void interpret_trailers(const struct process_trailer_options *opts,
> if (opts->in_place)
> outfile = create_in_place_tempfile(file);
>
> - parse_trailers(opts, &info, sb.buf, &head);
> + info = parse_trailers(opts, sb.buf, &head);
I think this patch might be doing too much at once and could have been
split into 3 or more patches to make reviews easier.
For example the first patch could introduce trailer_info_new() and
make interpret_trailers() use it. Then the second patch could modify
parse_trailers() so that it returns a 'struct trailer_info *'. Then
the third patch could make the trailer_info struct private.
^ permalink raw reply
* Re: [PATCH v4 21/28] trailer: spread usage of "trailer_block" language
From: Christian Couder @ 2024-02-12 23:39 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <38f4b4c4135dfebc06c2b1d5c56854af4b07fedc.1707196348.git.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Linus Arver <linusa@google.com>
>
> Deprecate the "trailer_info" struct name and replace it with
> "trailer_block". The main reason is to help readability, because
> "trailer_info" on the surface sounds like it's about a single trailer
> when in reality it is a collection of contiguous lines, at least 25% of
> which are trailers.
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> builtin/interpret-trailers.c | 25 +++++-----
> trailer.c | 97 ++++++++++++++++++------------------
> trailer.h | 18 +++----
> 3 files changed, 71 insertions(+), 69 deletions(-)
>
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 6bf8cec005a..f76841c5280 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -140,8 +140,8 @@ static void interpret_trailers(const struct process_trailer_options *opts,
> {
> LIST_HEAD(head);
> struct strbuf sb = STRBUF_INIT;
> - struct strbuf trailer_block = STRBUF_INIT;
> - struct trailer_info *info;
> + struct strbuf tb = STRBUF_INIT;
> + struct trailer_block *trailer_block;
I understand that using 'trailer_block' for a 'struct trailer_block *'
makes sense and I like the idea behind this patch, but it's
unfortunate that 'struct strbuf trailer_block' becomes 'struct strbuf
tb'. Also the name change for 'struct strbuf trailer_block' could be
in a separate patch.
^ permalink raw reply
* Re: [PATCH v4 22/28] trailer: prepare to delete "parse_trailers_from_command_line_args()"
From: Christian Couder @ 2024-02-12 23:39 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <94bf182e3ffbf8ed6e20cd77b2e46e5b83c44d34.1707196348.git.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Linus Arver <linusa@google.com>
>
> Expose more functions in the trailer.h API, in preparation for deleting
>
> parse_trailers_from_command_line_args()
>
> from the trailers implementation, because it should not be concerned
> with command line arguments (as they have nothing to do with trailers
> themselves). Indeed, the interpret-trailers builtin is the only caller
> of this function inside Git.
>
> Rename add_arg_item() to trailer_add_arg_item() to expose it as an API
> function. Rename new_trailers_clear() to free_new_trailers() because it
> will be promoted into an API function; the API already has
> free_trailers(), so using the "free_*" naming style will keep it
> consistent. Also rename "conf_info" to "trailer_conf" for readability,
> dropping the low-value "_info" suffix as we did earlier in this series
> for "trailer_info" to "trailer_block".
That's a lot done in a single patch. I think splitting it into 3 or
more patches would be nice for reviewers.
^ permalink raw reply
* Re: [PATCH v4 23/28] trailer: add new helper functions to API
From: Christian Couder @ 2024-02-12 23:39 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <3bfe4809ecbc5aa0ea52daee7684289398cb88d4.1707196348.git.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Linus Arver <linusa@google.com>
>
> This is a preparatory refactor for deprecating "new_trailer_item" from
> the API (which will let us deprecate
> parse_trailers_from_command_line_args()).
>
> Expose new helper functions from the API, because we'll be calling them
> from interpret-trailers.c soon when we move
> parse_trailers_from_command_line_args() there.
>
> Move free_new_trailers() from the builtin to trailer.c because later on
> we will adjust it to free arg_item structs, which are private to
> trailer.c.
This patch seems to be also doing too much.
^ permalink raw reply
* Re: [PATCH v4 26/28] trailer: unify "--trailer ..." arg handling
From: Christian Couder @ 2024-02-12 23:39 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <9720526dd8a63b916c75fe9d6322ee13c8b36621.1707196348.git.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Linus Arver <linusa@google.com>
>
> Move the logic of parse_trailer_from_command_line_arg() into
> option_parse_trailer(), because that is the only caller and there's no
> benefit in keeping these two separate.
Well one benefit could be that 2 small functions might be easier to
understand than a big one. So perhaps
parse_trailer_from_command_line_arg() could just have been made
static?
^ permalink raw reply
* Re: [PATCH v4 27/28] trailer_set_*(): put out parameter at the end
From: Christian Couder @ 2024-02-12 23:39 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <26df2514acbf4d51f40f4b1b9f33a357fa424ac7.1707196348.git.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Linus Arver <linusa@google.com>
>
> The new trailer_config_set_*() functions which were introduced a few
> patches ago put the out parameter (the variable being mutated) at the
> end of the parameter list.
>
> Put the out parameter at the end for these functions for these existing
s/for these functions for these/for the/
> trailer_set_*() functions for consistency. This also avoids confusion
> for API users because otherwise these two sets of functions look rather
> similar in <trailer.h> even though they have completely different out
> parameters.
^ permalink raw reply
* Re: [PATCH v4 28/28] trailer: introduce "template" term for readability
From: Christian Couder @ 2024-02-12 23:40 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <14927038d855020f9ae7594ad5cc646257613cc1.1707196348.git.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Linus Arver <linusa@google.com>
>
> The term "arg_item" is ambiguous because we use it to hold data for
>
> (1) trailers specified as command line arguments (in
> builtin/interpret-trailers.c), and
>
> (2) trailers specified in configuration,
>
> and these are both used to ultimately insert new trailers (based
> on the contents of arg_item, acting as a kind of template) into some
> other set of existing trailers (such as those found in a trailer block
> inside a log message) that have already been parsed.
>
> Rename "arg_item" to "trailer_template". This necessitates further
> renames to make the functions that act on these templates match the data
> structures (parameters) they act on:
>
> - [*] add_arg_to_input_list() to apply_template_to_trailers()
> - [*] apply_arg_if_exists() to maybe_add_if_exists()
> - [*] apply_arg_if_missing() to maybe_add_if_missing()
> - apply_command() to run_command_from_template()
> - [*] apply_item_command() to populate_template_value()
> - free_arg_item() to free_template() (non-API function)
> - free_new_trailers() to free_trailer_templates() (API function)
> - get_conf_item() to get_or_add_template_by()
> - option_parse_trailer() to option_parse_trailer_template()
> - parse_trailers_from_config() to parse_trailer_templates_from_config()
> - [*] process_trailers_lists() to apply_trailer_templates()
> - token_from_item() to token_from_template()
> - token_matches_item to token_matches_template
> - [*] trailer_add_arg_item() to add_trailer_template()
> - trailer_from_arg() to trailer_from()
> - [*] check_if_different() (reorder parameters only)
> - [*] find_same_and_apply_arg() (reorder parameters only)
>
> Reorder parameters to prefer input parameters toward the beginning and
> out parameters at the end; these functions have been marked with an
> asterisk ([*]).
That's a lot of changes in a single patch.
> This removes the "arg" terminology (standing for "CLI arguments") from
> the trailer implementation, which makes sense because trailers
> themselves have nothing to do with CLI argument handling.
>
> Also note that these renames expose the previously liberal use of
> "trailer" to mean both trailers we read from the input text (trailer
> block) and trailer templates that are defined as CLI args or
> configurations. Some functions implied a single action when they could
> do two different things, so introduce words like "maybe" and "or" to
> make their behavior more explicit.
>
> In summary this patch renames and reorders parameters for readability,
> without any behavioral change. We don't rename
> find_same_and_apply_arg(), because it will be refactored soon.
>
> For parse_trailers_from_config() (renamed to
> parse_trailer_templates_from_config()), add a NEEDSWORK discussion about
> how the deprecated trailer.*.command configuration option is oddly more
> featureful than trailer.*.cmd (if we were to remove support for
> trailer.*.command, users would not be able to replicate the behavior
> with trailer.*.cmd and would lose out on functionality).
This change could be in a separate patch. Also there were discussions
when trailer.*.command was deprecated and trailer.*.cmd introduced. I
think it might be useful to talk about them in the commit message of
the separate patch introducing the NEEDSWORK.
^ permalink raw reply
* [PATCH v6 0/2] add-patch: classify '@' as a synonym for 'HEAD'
From: Ghanshyam Thakkar @ 2024-02-13 0:05 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, sunshine, ps, Ghanshyam Thakkar
In-Reply-To: <20240211202035.7196-2-shyamthakkar001@gmail.com>
I just noticed after sending v5 that, in one of the tests, while
moving it inside the loop for also testing '@', set_and_save_state was
not used therefore the subsequent tests after the first $opt will not have
the changes for which we prove 'y' thereby making the tests useless.
(Although it would not fail regardless of this). This got unnoticed by
the previous reviews since v1 and me as well.
Apologies for the oversight and noise.
Ghanshyam Thakkar (2):
add-patch: classify '@' as a synonym for 'HEAD'
add -p tests: remove PERL prerequisites
add-patch.c | 8 ------
builtin/checkout.c | 4 ++-
builtin/reset.c | 4 ++-
t/t2016-checkout-patch.sh | 46 +++++++++++++++++++---------------
t/t2020-checkout-detach.sh | 12 +++++++++
t/t2024-checkout-dwim.sh | 2 +-
t/t2071-restore-patch.sh | 46 ++++++++++++++++++----------------
t/t3904-stash-patch.sh | 6 -----
t/t7105-reset-patch.sh | 38 +++++++++++++++-------------
t/t7106-reset-unborn-branch.sh | 2 +-
t/t7514-commit-patch.sh | 6 -----
11 files changed, 92 insertions(+), 82 deletions(-)
change since v5:
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 0f597416d8..453872c8ba 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -29,6 +29,7 @@ test_expect_success PERL 'saying "n" does nothing' '
for opt in "HEAD" "@" ""
do
test_expect_success PERL "git reset -p $opt" '
+ set_and_save_state dir/foo work work &&
test_write_lines n y | git reset -p $opt >output &&
verify_state dir/foo work head &&
verify_saved_state bar &&
--
2.43.0
^ permalink raw reply related
* [PATCH v6 1/2] add-patch: classify '@' as a synonym for 'HEAD'
From: Ghanshyam Thakkar @ 2024-02-13 0:05 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, sunshine, ps, Ghanshyam Thakkar
In-Reply-To: <20240211202035.7196-2-shyamthakkar001@gmail.com>
Currently, (restore, checkout, reset) commands correctly take '@' as a
synonym for 'HEAD'. However, in patch mode different prompts/messages
are given on command line due to patch mode machinery not considering
'@' to be a synonym for 'HEAD' due to literal string comparison with
the word 'HEAD', and therefore assigning patch_mode_($command)_nothead
and triggering reverse mode (-R in diff-index). The NEEDSWORK comment
suggested comparing commit objects to get around this. However, doing
so would also take a non-checked out branch pointing to the same commit
as HEAD, as HEAD. This would cause confusion to the user.
Therefore, after parsing '@', replace it with 'HEAD' as reasonably
early as possible. This also solves another problem of disparity
between 'git checkout HEAD' and 'git checkout @' (latter detaches at
the HEAD commit and the former does not).
Trade-offs:
- Some of the errors would show the revision argument as 'HEAD' when
given '@'. This should be fine, as most users who probably use '@'
would be aware that it is a shortcut for 'HEAD' and most probably
used to use 'HEAD'. There is also relevant documentation in
'gitrevisions' manpage about '@' being the shortcut for 'HEAD'. Also,
the simplicity of the solution far outweighs this cost.
- Consider '@' as a shortcut for 'HEAD' even if 'refs/heads/@' exists
at a different commit. Naming a branch '@' is an obvious foot-gun and
many existing commands already take '@' for 'HEAD' even if
'refs/heads/@' exists at a different commit or does not exist at all
(e.g. 'git log @', 'git push origin @' etc.). Therefore this is an
existing assumption and should not be a problem.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
add-patch.c | 8 -------
builtin/checkout.c | 4 +++-
builtin/reset.c | 4 +++-
t/t2016-checkout-patch.sh | 46 +++++++++++++++++++++-----------------
t/t2020-checkout-detach.sh | 12 ++++++++++
t/t2071-restore-patch.sh | 18 +++++++++------
t/t7105-reset-patch.sh | 16 ++++++++-----
7 files changed, 65 insertions(+), 43 deletions(-)
diff --git a/add-patch.c b/add-patch.c
index 79eda168eb..68f525b35c 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1729,14 +1729,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
if (mode == ADD_P_STASH)
s.mode = &patch_mode_stash;
else if (mode == ADD_P_RESET) {
- /*
- * NEEDSWORK: Instead of comparing to the literal "HEAD",
- * compare the commit objects instead so that other ways of
- * saying the same thing (such as "@") are also handled
- * appropriately.
- *
- * This applies to the cases below too.
- */
if (!revision || !strcmp(revision, "HEAD"))
s.mode = &patch_mode_reset_head;
else
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a6e30931b5..067c251933 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1224,7 +1224,9 @@ static void setup_new_branch_info_and_source_tree(
struct tree **source_tree = &opts->source_tree;
struct object_id branch_rev;
- new_branch_info->name = xstrdup(arg);
+ /* treat '@' as a shortcut for 'HEAD' */
+ new_branch_info->name = !strcmp(arg, "@") ? xstrdup("HEAD") :
+ xstrdup(arg);
setup_branch_path(new_branch_info);
if (!check_refname_format(new_branch_info->path, 0) &&
diff --git a/builtin/reset.c b/builtin/reset.c
index 8390bfe4c4..f0bf29a478 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -281,7 +281,9 @@ static void parse_args(struct pathspec *pathspec,
verify_filename(prefix, argv[0], 1);
}
}
- *rev_ret = rev;
+
+ /* treat '@' as a shortcut for 'HEAD' */
+ *rev_ret = !strcmp("@", rev) ? "HEAD" : rev;
parse_pathspec(pathspec, 0,
PATHSPEC_PREFER_FULL |
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index 747eb5563e..c4f9bf09aa 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' '
verify_state dir/foo index index
'
-test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '
- set_and_save_state dir/foo work head &&
- test_write_lines n y n | git checkout -p HEAD &&
- verify_saved_state bar &&
- verify_saved_state dir/foo
-'
-
-test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '
- test_write_lines n y y | git checkout -p HEAD &&
- verify_saved_state bar &&
- verify_state dir/foo head head
-'
-
-test_expect_success 'git checkout -p HEAD with change already staged' '
- set_state dir/foo index index &&
- # the third n is to get out in case it mistakenly does not apply
- test_write_lines n y n | git checkout -p HEAD &&
- verify_saved_state bar &&
- verify_state dir/foo head head
-'
+for opt in "HEAD" "@"
+do
+ test_expect_success "git checkout -p $opt with NO staged changes: abort" '
+ set_and_save_state dir/foo work head &&
+ test_write_lines n y n | git checkout -p $opt >output &&
+ verify_saved_state bar &&
+ verify_saved_state dir/foo &&
+ test_grep "Discard" output
+ '
+
+ test_expect_success "git checkout -p $opt with NO staged changes: apply" '
+ test_write_lines n y y | git checkout -p $opt >output &&
+ verify_saved_state bar &&
+ verify_state dir/foo head head &&
+ test_grep "Discard" output
+ '
+
+ test_expect_success "git checkout -p $opt with change already staged" '
+ set_state dir/foo index index &&
+ # the third n is to get out in case it mistakenly does not apply
+ test_write_lines n y n | git checkout -p $opt >output &&
+ verify_saved_state bar &&
+ verify_state dir/foo head head &&
+ test_grep "Discard" output
+ '
+done
test_expect_success 'git checkout -p HEAD^...' '
# the third n is to get out in case it mistakenly does not apply
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 8202ef8c74..bce284c297 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -45,6 +45,18 @@ test_expect_success 'checkout branch does not detach' '
check_not_detached
'
+for opt in "HEAD" "@"
+do
+ test_expect_success "checkout $opt no-op/don't detach" '
+ reset &&
+ cat .git/HEAD >expect &&
+ git checkout $opt &&
+ cat .git/HEAD >actual &&
+ check_not_detached &&
+ test_cmp expect actual
+ '
+done
+
test_expect_success 'checkout tag detaches' '
reset &&
git checkout tag &&
diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
index b5c5c0ff7e..3dc9184b4a 100755
--- a/t/t2071-restore-patch.sh
+++ b/t/t2071-restore-patch.sh
@@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '
verify_state dir/foo index index
'
-test_expect_success PERL 'git restore -p --source=HEAD' '
- set_state dir/foo work index &&
- # the third n is to get out in case it mistakenly does not apply
- test_write_lines n y n | git restore -p --source=HEAD &&
- verify_saved_state bar &&
- verify_state dir/foo head index
-'
+for opt in "HEAD" "@"
+do
+ test_expect_success PERL "git restore -p --source=$opt" '
+ set_state dir/foo work index &&
+ # the third n is to get out in case it mistakenly does not apply
+ test_write_lines n y n | git restore -p --source=$opt >output &&
+ verify_saved_state bar &&
+ verify_state dir/foo head index &&
+ test_grep "Discard" output
+ '
+done
test_expect_success PERL 'git restore -p --source=HEAD^' '
set_state dir/foo work index &&
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 05079c7246..453872c8ba 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -26,12 +26,16 @@ test_expect_success PERL 'saying "n" does nothing' '
verify_saved_state bar
'
-test_expect_success PERL 'git reset -p' '
- test_write_lines n y | git reset -p >output &&
- verify_state dir/foo work head &&
- verify_saved_state bar &&
- test_grep "Unstage" output
-'
+for opt in "HEAD" "@" ""
+do
+ test_expect_success PERL "git reset -p $opt" '
+ set_and_save_state dir/foo work work &&
+ test_write_lines n y | git reset -p $opt >output &&
+ verify_state dir/foo work head &&
+ verify_saved_state bar &&
+ test_grep "Unstage" output
+ '
+done
test_expect_success PERL 'git reset -p HEAD^' '
test_write_lines n y | git reset -p HEAD^ >output &&
--
2.43.0
^ permalink raw reply related
* [PATCH v6 2/2] add -p tests: remove PERL prerequisites
From: Ghanshyam Thakkar @ 2024-02-13 0:05 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, sunshine, ps, Ghanshyam Thakkar
In-Reply-To: <20240211202035.7196-2-shyamthakkar001@gmail.com>
The Perl version of the add -i/-p commands has been removed since
20b813d (add: remove "add.interactive.useBuiltin" & Perl "git
add--interactive", 2023-02-07)
Therefore, Perl prerequisite in the test scripts which use the patch
mode functionality is not neccessary.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t2024-checkout-dwim.sh | 2 +-
t/t2071-restore-patch.sh | 28 ++++++++++++++--------------
t/t3904-stash-patch.sh | 6 ------
t/t7105-reset-patch.sh | 22 +++++++++++-----------
t/t7106-reset-unborn-branch.sh | 2 +-
t/t7514-commit-patch.sh | 6 ------
6 files changed, 27 insertions(+), 39 deletions(-)
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index a97416ce65..a3b1449ef1 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -113,7 +113,7 @@ test_expect_success 'checkout of branch from multiple remotes fails with advice'
test_grep ! "^hint: " stderr
'
-test_expect_success PERL 'checkout -p with multiple remotes does not print advice' '
+test_expect_success 'checkout -p with multiple remotes does not print advice' '
git checkout -B main &&
test_might_fail git branch -D foo &&
diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
index 3dc9184b4a..27e85be40a 100755
--- a/t/t2071-restore-patch.sh
+++ b/t/t2071-restore-patch.sh
@@ -4,7 +4,7 @@ test_description='git restore --patch'
. ./lib-patch-mode.sh
-test_expect_success PERL 'setup' '
+test_expect_success 'setup' '
mkdir dir &&
echo parent >dir/foo &&
echo dummy >bar &&
@@ -16,28 +16,28 @@ test_expect_success PERL 'setup' '
save_head
'
-test_expect_success PERL 'restore -p without pathspec is fine' '
+test_expect_success 'restore -p without pathspec is fine' '
echo q >cmd &&
git restore -p <cmd
'
# note: bar sorts before dir/foo, so the first 'n' is always to skip 'bar'
-test_expect_success PERL 'saying "n" does nothing' '
+test_expect_success 'saying "n" does nothing' '
set_and_save_state dir/foo work head &&
test_write_lines n n | git restore -p &&
verify_saved_state bar &&
verify_saved_state dir/foo
'
-test_expect_success PERL 'git restore -p' '
+test_expect_success 'git restore -p' '
set_and_save_state dir/foo work head &&
test_write_lines n y | git restore -p &&
verify_saved_state bar &&
verify_state dir/foo head head
'
-test_expect_success PERL 'git restore -p with staged changes' '
+test_expect_success 'git restore -p with staged changes' '
set_state dir/foo work index &&
test_write_lines n y | git restore -p &&
verify_saved_state bar &&
@@ -46,7 +46,7 @@ test_expect_success PERL 'git restore -p with staged changes' '
for opt in "HEAD" "@"
do
- test_expect_success PERL "git restore -p --source=$opt" '
+ test_expect_success "git restore -p --source=$opt" '
set_state dir/foo work index &&
# the third n is to get out in case it mistakenly does not apply
test_write_lines n y n | git restore -p --source=$opt >output &&
@@ -56,7 +56,7 @@ do
'
done
-test_expect_success PERL 'git restore -p --source=HEAD^' '
+test_expect_success 'git restore -p --source=HEAD^' '
set_state dir/foo work index &&
# the third n is to get out in case it mistakenly does not apply
test_write_lines n y n | git restore -p --source=HEAD^ &&
@@ -64,7 +64,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^' '
verify_state dir/foo parent index
'
-test_expect_success PERL 'git restore -p --source=HEAD^...' '
+test_expect_success 'git restore -p --source=HEAD^...' '
set_state dir/foo work index &&
# the third n is to get out in case it mistakenly does not apply
test_write_lines n y n | git restore -p --source=HEAD^... &&
@@ -72,7 +72,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^...' '
verify_state dir/foo parent index
'
-test_expect_success PERL 'git restore -p handles deletion' '
+test_expect_success 'git restore -p handles deletion' '
set_state dir/foo work index &&
rm dir/foo &&
test_write_lines n y | git restore -p &&
@@ -85,21 +85,21 @@ test_expect_success PERL 'git restore -p handles deletion' '
# dir/foo. There's always an extra 'n' to reject edits to dir/foo in
# the failure case (and thus get out of the loop).
-test_expect_success PERL 'path limiting works: dir' '
+test_expect_success 'path limiting works: dir' '
set_state dir/foo work head &&
test_write_lines y n | git restore -p dir &&
verify_saved_state bar &&
verify_state dir/foo head head
'
-test_expect_success PERL 'path limiting works: -- dir' '
+test_expect_success 'path limiting works: -- dir' '
set_state dir/foo work head &&
test_write_lines y n | git restore -p -- dir &&
verify_saved_state bar &&
verify_state dir/foo head head
'
-test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
+test_expect_success 'path limiting works: HEAD^ -- dir' '
set_state dir/foo work head &&
# the third n is to get out in case it mistakenly does not apply
test_write_lines y n n | git restore -p --source=HEAD^ -- dir &&
@@ -107,7 +107,7 @@ test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
verify_state dir/foo parent head
'
-test_expect_success PERL 'path limiting works: foo inside dir' '
+test_expect_success 'path limiting works: foo inside dir' '
set_state dir/foo work head &&
# the third n is to get out in case it mistakenly does not apply
test_write_lines y n n | (cd dir && git restore -p foo) &&
@@ -115,7 +115,7 @@ test_expect_success PERL 'path limiting works: foo inside dir' '
verify_state dir/foo head head
'
-test_expect_success PERL 'none of this moved HEAD' '
+test_expect_success 'none of this moved HEAD' '
verify_saved_head
'
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index accfe3845c..368fc2a6cc 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -3,12 +3,6 @@
test_description='stash -p'
. ./lib-patch-mode.sh
-if ! test_have_prereq PERL
-then
- skip_all='skipping stash -p tests, perl not available'
- test_done
-fi
-
test_expect_success 'setup' '
mkdir dir &&
echo parent > dir/foo &&
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 453872c8ba..f4f3b7a677 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -5,7 +5,7 @@ test_description='git reset --patch'
TEST_PASSES_SANITIZE_LEAK=true
. ./lib-patch-mode.sh
-test_expect_success PERL 'setup' '
+test_expect_success 'setup' '
mkdir dir &&
echo parent > dir/foo &&
echo dummy > bar &&
@@ -19,7 +19,7 @@ test_expect_success PERL 'setup' '
# note: bar sorts before foo, so the first 'n' is always to skip 'bar'
-test_expect_success PERL 'saying "n" does nothing' '
+test_expect_success 'saying "n" does nothing' '
set_and_save_state dir/foo work work &&
test_write_lines n n | git reset -p &&
verify_saved_state dir/foo &&
@@ -28,7 +28,7 @@ test_expect_success PERL 'saying "n" does nothing' '
for opt in "HEAD" "@" ""
do
- test_expect_success PERL "git reset -p $opt" '
+ test_expect_success "git reset -p $opt" '
set_and_save_state dir/foo work work &&
test_write_lines n y | git reset -p $opt >output &&
verify_state dir/foo work head &&
@@ -37,28 +37,28 @@ do
'
done
-test_expect_success PERL 'git reset -p HEAD^' '
+test_expect_success 'git reset -p HEAD^' '
test_write_lines n y | git reset -p HEAD^ >output &&
verify_state dir/foo work parent &&
verify_saved_state bar &&
test_grep "Apply" output
'
-test_expect_success PERL 'git reset -p HEAD^^{tree}' '
+test_expect_success 'git reset -p HEAD^^{tree}' '
test_write_lines n y | git reset -p HEAD^^{tree} >output &&
verify_state dir/foo work parent &&
verify_saved_state bar &&
test_grep "Apply" output
'
-test_expect_success PERL 'git reset -p HEAD^:dir/foo (blob fails)' '
+test_expect_success 'git reset -p HEAD^:dir/foo (blob fails)' '
set_and_save_state dir/foo work work &&
test_must_fail git reset -p HEAD^:dir/foo &&
verify_saved_state dir/foo &&
verify_saved_state bar
'
-test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
+test_expect_success 'git reset -p aaaaaaaa (unknown fails)' '
set_and_save_state dir/foo work work &&
test_must_fail git reset -p aaaaaaaa &&
verify_saved_state dir/foo &&
@@ -70,27 +70,27 @@ test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
# dir/foo. There's always an extra 'n' to reject edits to dir/foo in
# the failure case (and thus get out of the loop).
-test_expect_success PERL 'git reset -p dir' '
+test_expect_success 'git reset -p dir' '
set_state dir/foo work work &&
test_write_lines y n | git reset -p dir &&
verify_state dir/foo work head &&
verify_saved_state bar
'
-test_expect_success PERL 'git reset -p -- foo (inside dir)' '
+test_expect_success 'git reset -p -- foo (inside dir)' '
set_state dir/foo work work &&
test_write_lines y n | (cd dir && git reset -p -- foo) &&
verify_state dir/foo work head &&
verify_saved_state bar
'
-test_expect_success PERL 'git reset -p HEAD^ -- dir' '
+test_expect_success 'git reset -p HEAD^ -- dir' '
test_write_lines y n | git reset -p HEAD^ -- dir &&
verify_state dir/foo work parent &&
verify_saved_state bar
'
-test_expect_success PERL 'none of this moved HEAD' '
+test_expect_success 'none of this moved HEAD' '
verify_saved_head
'
diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh
index d20e5709f9..88d1c8adf4 100755
--- a/t/t7106-reset-unborn-branch.sh
+++ b/t/t7106-reset-unborn-branch.sh
@@ -34,7 +34,7 @@ test_expect_success 'reset $file' '
test_cmp expect actual
'
-test_expect_success PERL 'reset -p' '
+test_expect_success 'reset -p' '
rm .git/index &&
git add a &&
echo y >yes &&
diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
index 998a2103c7..b4de10a5dd 100755
--- a/t/t7514-commit-patch.sh
+++ b/t/t7514-commit-patch.sh
@@ -3,12 +3,6 @@
test_description='hunk edit with "commit -p -m"'
. ./test-lib.sh
-if ! test_have_prereq PERL
-then
- skip_all="skipping '$test_description' tests, perl not available"
- test_done
-fi
-
test_expect_success 'setup (initial)' '
echo line1 >file &&
git add file &&
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v4 00/28] Enrich Trailer API
From: Junio C Hamano @ 2024-02-13 0:11 UTC (permalink / raw)
To: Christian Couder
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Emily Shaffer, Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <CAP8UFD3qLGua5PTA+i29N+yJH8iKVBPwUn=S606B2E+s959JFQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> This patch series is the first 10 patches of a larger cleanup/bugfix series
>> (henceforth "larger series") I've been working on.
>
> There are now 28 patches in this series.
>
> I took a look at all of them, and I think that this series should be
> split into 4 or more series.
I presume that [01-09/28] would be the first part, nothing
controversial and consisting of obvious clean-ups? I do not mind
merging that part down to remove the future review load if everybody
agrees.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/1] completion: dir-type optargs for am, format-patch
From: Britton Kerin @ 2024-02-13 0:52 UTC (permalink / raw)
To: Rubén Justo, git
In-Reply-To: <40c3a824-a961-490b-94d4-4eb23c8f713d@gmail.com>
On Sat, Feb 3, 2024 at 6:13 AM Rubén Justo <rjusto@gmail.com> wrote:
>
> On 08-ene-2024 15:53:03, Britton Leo Kerin wrote:
> > Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
> > ---
> > contrib/completion/git-completion.bash | 37 ++++++++++++++++++++++++++
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index 185b47d802..2b2b6c9738 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -1356,6 +1356,29 @@ __git_count_arguments ()
> > printf "%d" $c
> > }
> >
> > +# Complete actual dir (not pathspec), respecting any -C options.
> > +#
> > +# Usage: __git_complete_refs [<option>]...
> > +# --cur=<word>: The current dir to be completed. Defaults to the current word.
> > +__git_complete_dir ()
> > +{
> > + local cur_="$cur"
> > +
> > + while test $# != 0; do
> > + case "$1" in
> > + --cur=*) cur_="${1##--cur=}" ;;
> > + *) return 1 ;;
> > + esac
> > + shift
> > + done
> > +
> > + # This rev-parse invocation amounts to a pwd which respects -C options
> > + local context_dir=$(__git rev-parse --show-toplevel --show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)
> > + [ -d "$context_dir" ] || return 1
> > +
> > + COMPREPLY=$(cd "$context_dir" 2>/dev/null && compgen -d -- "$cur_")
>
> This assignment is problematic.
>
> First, COMPREPLY is expected to be an array. Maybe a simple change can
> do the trick:
>
> - COMPREPLY=$(cd "$context_dir" 2>/dev/null && compgen -d -- "$cur_")
> + COMPREPLY=( $(cd "$context_dir" 2>/dev/null && compgen -d -- "$cur_") )
>
> But, what happens with directories that have SP's in its name? We're
> giving wrong options:
>
> $ mkdir one
> $ mkdir "one more dir"
> $ git am --directory=o<TAB><TAB>
> dir more one
>
> Setting IFS can help us:
>
> + local IFS=$'\n'
>
> Now we're returning correct options:
>
> $ mkdir one
> $ mkdir "one more dir"
> $ git am --directory=o<TAB><TAB>
> one one more dir
>
> Here, the user might be expecting directory names with a trailing '/',
> as Bash do. Again, a simple trick:
>
> - COMPREPLY=( $(cd "$context_dir" 2>/dev/null && compgen -d -- "$cur_") )
> + COMPREPLY=( $(cd "$context_dir" 2>/dev/null && compgen -d -S / -- "$cur_") )
>
> Now looks better, IMO:
>
> $ git am --directory=o<TAB><TAB>
> one/ one more dir/
>
> But, after all of this, we're going to provoke a problematic completion due
> to the SP:
>
> $ mkdir "another one"
> $ git am --directory=anot<TAB><TAB>
> ...
> $ git am --directory=another one/
>
> We should complete to:
>
> $ git am --directory=another\ one/
>
> Here we need a less simple trick:
>
> + # use a hack to enable file mode in bash < 4
> + # compopt -o filenames +o nospace 2>/dev/null ||
> + compgen -f /non-existing-dir/ >/dev/null ||
> + true
>
> Some commits you may find interesting:
> fea16b47b6 (git-completion.bash: add support for path completion, 2013-01-11)
> 3ffa4df4b2 (completion: add hack to enable file mode in bash < 4, 2013-04-27)
>
> Well, so far, so good? I'm afraid, not: What happens with other
> special characters like quotes '"'?
>
> I suggest you take a look at how we are already doing all of
> considerations for other commands, like git-add.
Thanks for all these suggestions. Considering them and working on it
some more I end up with this function:
__git_complete_dir ()
{
local cur_="$cur"
while test $# != 0; do
case "$1" in
--cur=*) cur_="${1##--cur=}" ;;
*) return 1 ;;
esac
shift
done
# This rev-parse invocation amounts to a pwd which respects -C options
local context_dir=$(__git rev-parse --show-toplevel
--show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)
[ -d "$context_dir" ] || return 1
compopt -o noquote
local IFS=$'\n'
local unescaped_candidates=($(cd "$context_dir" 2>/dev/null &&
compgen -d -S / -- "$cur_"))
for ii in "${!unescaped_candidates[@]}"; do
COMPREPLY[$ii]=$(printf "%q" "${unescaped_candidates[$ii]}")
done
}
This one works for all weird characters that I've tried in bash 5.2 at
least, and in frameworks that do their own escaping also (e.g.
ble.sh). Since your advice so far was so good I thought I'd ask if
there is anything obvious to you that is still wrong here?
If not I guess what's left is special code to make it work better with
old versions of bash. I'm a little sceptical that this is worth it
since bash 5 is already 5 years old and it's only completion code
we're talking about but I guess it could be done.
Britton
^ permalink raw reply
* Re: libification: how to avoid symbol collisions?
From: Kyle Lippincott @ 2024-02-13 1:02 UTC (permalink / raw)
To: rsbecker; +Cc: git
In-Reply-To: <000301da5b5c$5477d7f0$fd6787d0$@nexbridge.com>
On Fri, Feb 9, 2024 at 5:31 AM <rsbecker@nexbridge.com> wrote:
>
> On Thursday, February 8, 2024 9:30 PM, Kyle Lippincott wrote:
> >While thinking about libification, I was wondering what we can/need to do about
> >symbols (specifically functions, since our libraries will likely have few to no extern
> >variables) that need to escape their translation unit (.c file) but that we don't want
> >to risk colliding with symbols from our "host" project.
> >
> >For any header that we're offering up as an API boundary we can have prefixed
> >names, but there are symbols from git-compat-util.h with simple and likely
> >common names like `die`, `usage`, error`, etc. I'm far from an expert on linkers, but
> >I'm under the impression that even though we'll only be #including git-compat-
> >util.h in our own .c files (so the compiler for the host project wouldn't see them),
> >the produced static library will still be "providing" these symbols unless we mark
> >them as `static` (and if we mark them as `static`, they can't be used outside of their
> >translation unit). This means that if the host project has a version of `die` (or links
> >against yet another library that does), we run into what C++ calls a One Definition
> >Rule (ODR)
> >violation: we have two providers of the symbol `die` with different
> >implementations, and the behavior is undefined (no error needs to be generated as
> >far as I know).
> >
> >With dynamic libraries I believe that we have more control over what gets exposed,
> >but I don't know of functionality for this when linking statically. I'm assuming there
> >is no such functionality, as projects like openssl (ty Randall for mentioning this)
> >appear to have a convention of prefixing the symbols they put in their "public" API
> >(i.e. in non-internal header files) with things like OSSL_, and of prefixing the symbols
> >they put in their "private" APIs that can't be marked as `static` with `ossl_`. I'd love
> >to be wrong about this. :)
> >
> >If I'm right that this is an issue, does this imply that we'd need to rename every non-
> >static function in the git codebase that's part of a library to have a `git_` prefix, even
> >if it won't be used outside of the git project's own .c files? Is there a solution that
> >doesn't involve making it so that we have to type `git_` a billion times a day that's
> >also maintainable? We could try to guess at how likely a name collision would be
> >and only do this for ones where it's obviously going to collide, but if we get it wrong,
> >I'm concerned that we'll run into subtle ODR violations that *at best* erode
> >confidence in our library, and can actually cause outages, data corruption, and
> >security/privacy issues.
>
> I think we only need to do this for functions that are in the libification code-base for non-static symbols (and any data elements that may end up in a DLL some day).
I believe the hope is that the majority/entirety of plumbing code will
be provided as a library, and we'll likely want to have a significant
portion of porcelain code as well. I think we're really talking about
(effectively) "all of git", but not all at once. If we attempt to make
things safe based on guesses about what's likely to collide with other
project's code, we'll (a) get it wrong, and only discover later when
they try to add our library to their project, and (b) have a
maintenance burden, where we now have to think about every function
name we introduce, which would not be fun (and we'll get it wrong.
Frequently.)
> The bulk of the non-libified code base would only need to adapt to new symbol names if those symbols are specifically moved.
I'm not following what you mean by "moved" here.
> die(), error(), are probably going to be impacted, but they can be aliased with #defines internally to git to git_die() or git_error(), for example.
> --Randall
>
^ permalink raw reply
* Possible bug with 'diff --cc' report
From: Groboclown @ 2024-02-13 2:24 UTC (permalink / raw)
To: git
While testing out the report parser for the gitleaks go-gitdiff
library against a repository, I encountered output in the form:
@@@ -229,4 -229,18446744073709551615 +228,3 @@@ Comment
The '18446744073709551615' value is interesting, in that it's a uint64
value storing a signed int64 -1. Along with this, the number of
changed lines in the output was actually 5 / 0 / 4 (as opposed to 4 /
-1 / 3). I found the '-1' value as the first, second, and third line
counts (the code only used at most 2 parents per commit).
At first I thought this to only happen on some 0 line change
instances, but I also found a diff result that looked like (I added a
' mark before each line to make the spacing obvious):
'@@@ -225,4 -237,2 +225,6 @@@ Comment
' +line 1
' +line 2
' +line 3
' +line 4
'- line 5
'++line 6
'++line 7
'+ line 8
' -line 9
' -line 10
The reported line counts doesn't match the actual output. Based on
other examples plus the documentation, it looks like these are also
off by 1 - by my count, it should be 5 / 3 / 7, rather than 4 / 2 / 6.
System information:
git version 2.43.0
cpu: x86_64
sizeof-long: 8
sizeof-size_t: 8
uname: Linux 5.15.146.1-microsoft-standard-WSL2
compiler info: gnuc: 13.2
libc info: glibc: 2.39
^ permalink raw reply
* Re: libification: how to avoid symbol collisions?
From: Kyle Lippincott @ 2024-02-13 2:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqil2ximxq.fsf@gitster.g>
On Fri, Feb 9, 2024 at 9:10 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Kyle Lippincott <spectral@google.com> writes:
>
> > If I'm right that this is an issue, does this imply that we'd need to
> > rename every non-static function in the git codebase that's part of a
> > library to have a `git_` prefix, even if it won't be used outside of
> > the git project's own .c files? Is there a solution that doesn't
> > involve making it so that we have to type `git_` a billion times a day
> > that's also maintainable? We could try to guess at how likely a name
> > collision would be and only do this for ones where it's obviously
> > going to collide, but if we get it wrong, I'm concerned that we'll run
> > into subtle ODR violations that *at best* erode confidence in our
> > library, and can actually cause outages, data corruption, and
> > security/privacy issues.
>
> If you end up with a helper function name "foo" that is defined in
> our X.c and used by our Y.c but is not part of the published "git
> library API", we may want to rename it so that such a common name
> can be used by programs that link with the "git library". We may
> choose to rename it to "GitLib_foo".
If it's internal, we may want to name it with a different prefix than
GitLib, if we expect the exposed API of the library to have this
prefix, just as a signal to readers where the internal/external
boundaries are.
>
> Do we want to keep the source of our library code, which defines the
> helper function "foo" in X.c and calls it in Y.c, intact so that the
> helper is still named "foo" as far as we are concerned? Or do we
> "bite the bullet" and bulk modify both the callers and the callee?
>
> I'd imagine that we would rather avoid such a churn at all cost [*].
> After all, "foo" is *not* supposed to be callable by any human
> written code, and that is why we rename it to a name "GitLib_foo"
> that is unlikely to overlap with any sane human would use.
>
> Side note: if a public API function that we want our library
> clients to call is misnamed, we want to rename it so that we
> would both internally and externally use the same public
> name, I would imagine.
>
> The mechanics to rename should be a solved problem, I think, as we
> are obviously not the first project that wants to build a library.
>
> If the names are all simple, we could do this in CPP,
At first I thought you meant C++, and I was like "Yeah, that's a
possible solution: when building a library, compile it as C++ with
name mangling, except for the symbols we intend to export!" -- this
was not what you meant, though. Kind of amusingly, that idea might
work, and might even be maintainable once we got to that state, but
getting to that state would be a lot of cleanup because of C++'s
stricter type system (`char *p = ptr;`, where `ptr` is a `void*` for
example; maybe this is a call to malloc or similar). Since the git
libraries don't exist yet, there's technically no worries about
backwards compatibility with requiring a C++ compiler.
> i.e. invent a
> header file that has bunch of such renames like
>
> #define foo GitLib_foo
>
> and include it in both X.c and Y.c. But "foo" may also appear as
> the name of a type, a member in a structure, etc., where we may not
> want to touch, so in a project larger than toy scale, this approach
> may not work well.
Glancing at the tags file, it looks like there's a small number of
cases where this would be problematic, and they're mostly things where
there's a function named the same thing as either a struct variable
storing the result of the function. So it could work, but there's over
3,500 symbols (if I did my filtering of the tags file correctly) that
are not scoped to a specific file (i.e. static), or
struct/enum/typedef/union names. That's going to be quite annoying to
maintain; even if we don't end up having to do all 3,500 symbols, for
the files that are part of some public library, we'd add maintenance
burden because we'd need to remember to either make every new function
be static, or add it to this list. I assume we could create a test
that would enforce this ("static, named with <prefix>, or added to
<list>"), so the issue is catchable, but it will be exceedingly
annoying every time one encounters this.
>
> "objcopy --redefine-sym" would probably be a better way. I haven't
> written a linker script, but I heard rumors that there is RENAME()
> to rename symbols, and using that might be another avenue.
>
>
I'd thought of linker scripts, but rejected the idea due to
assumptions I made about their portability - this could be mitigated
by having a linker-script-generator step in the build process, but
this seems difficult to maintain. It also implies the same maintenance
burden as the #defines, where when introducing a new function to X.c
that is called from Y.c we'd have to edit the list of "symbols to
rename".
^ permalink raw reply
* Re: [PATCH v4 00/28] Enrich Trailer API
From: Christian Couder @ 2024-02-13 6:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Emily Shaffer, Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <xmqqjzn94404.fsf@gitster.g>
On Tue, Feb 13, 2024 at 1:11 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> This patch series is the first 10 patches of a larger cleanup/bugfix series
> >> (henceforth "larger series") I've been working on.
> >
> > There are now 28 patches in this series.
> >
> > I took a look at all of them, and I think that this series should be
> > split into 4 or more series.
>
> I presume that [01-09/28] would be the first part, nothing
> controversial and consisting of obvious clean-ups? I do not mind
> merging that part down to remove the future review load if everybody
> agrees.
Yeah, patches [01-09/28] look good to me. If we are merging them
without the rest, we might want to change a bit the last sentence in
09/28 which says:
"In the next patch, we'll change format_trailer_info() to use the
parsed trailer_item objects instead of the string array."
Maybe just: s/In the next patch/In a future patch/
^ permalink raw reply
* Re: [PATCH v2 7/7] reftable/reader: add comments to `table_iter_next()`
From: Patrick Steinhardt @ 2024-02-13 6:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine, John Cai
In-Reply-To: <xmqq4jed7g87.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 916 bytes --]
On Mon, Feb 12, 2024 at 09:19:20AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > While working on the optimizations in the preceding patches I stumbled
> > upon `table_iter_next()` multiple times. It is quite easy to miss the
> > fact that we don't call `table_iter_next_in_block()` twice, but that the
> > second call is in fact `table_iter_next_block()`.
> >
> > Add comments to explain what exactly is going on here to make things
> > more obvious. While at it, touch up the code to conform to our code
> > style better.
> >
> > Note that one of the refactorings merges two conditional blocks into
> > one. Before, we had the following code:
> >
> > ```
> > err = table_iter_next_block(&next, ti
>
> ");"???
Oh, right, seems to be a copy-paste error. I see you already fixed it in
ps/reftable-iteration-perf and merged the branch to next. Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper
From: Jeff King @ 2024-02-13 7:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Steadmon, git, johannes.schindelin, phillip.wood
In-Reply-To: <xmqq34tx5q6o.fsf@gitster.g>
On Mon, Feb 12, 2024 at 01:27:11PM -0800, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > I see this line in the docs [1]: "As with wildcard expansion in rules,
> > the results of the wildcard function are sorted". GNU Make has restored
> > the sorted behavior of $(wildcard) since 2018 [2]. I'll leave the sort
> > off for now, but if folks feel like we need to support older versions of
> > `make`, I'll add it back.
> >
> > [1] https://www.gnu.org/software/make/manual/html_node/Wildcard-Function.html
> > [2] https://savannah.gnu.org/bugs/index.php?52076
>
> Thanks for digging. I thought I was certain that woldcard is sorted
> and stable and was quite perplexed when I could not find the mention
> in a version of doc I had handy ("""This is Edition 0.75, last
> updated 19 January 2020, of 'The GNU Make Manual', for GNU 'make'
> version 4.3.""").
Likewise (mine is the latest version in Debian unstable). The change to
sort comes from their[1] eedea52a, which was in GNU make 4.2.90. But the
matching documentation change didn't happen until 5b993ae, which was
4.3.90 in late 2021. So that explains the mystery.
Those dates imply to me that we should keep the $(sort), though. Six
years is not so long in distro timescales, especially given that Debian
unstable is on a 4-year-old version. (And if we did want to get rid of
it, certainly we should do so consistently across the Makefile in a
separate patch).
-Peff
[1] commit ids are from https://git.savannah.gnu.org/git/make.git
^ permalink raw reply
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Jeff King @ 2024-02-13 8:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Maarten Bosmans, git, Teng Long, Maarten Bosmans
In-Reply-To: <xmqqy1bxiiop.fsf@gitster.g>
On Tue, Feb 06, 2024 at 09:52:38AM -0800, Junio C Hamano wrote:
> > That is also a cool idea. That would probably use the functionality of
> > the cat-file batch mode, right?
>
> Not really. I was hoping that "git show" that can take multiple
> objects from its command line would directly be used, or with a new
> option that gives a separator between these objects.
How about:
cat some_commit_ids |
git show --stdin -s -z --format='%H%n%N'
?
-Peff
^ 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