* Re: [PATCH 04/10] trailer: delete obsolete formatting functions
From: Junio C Hamano @ 2024-01-19 0:31 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder, Linus Arver
In-Reply-To: <8d86461475765ac04ad0ed207e46598eb90a45ba.1704869487.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> trailer.c | 79 -------------------------------------------------------
> 1 file changed, 79 deletions(-)
OK, these unused functions are the reason why 03/10 weren't removing
as many lines as I would have expected from the refactoring.
Looking good.
^ permalink raw reply
* Re: [PATCH 01/10] trailer: move process_trailers() to interpret-trailers.c
From: Linus Arver @ 2024-01-19 0:21 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder
In-Reply-To: <xmqqle8mp9bx.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> I am assuming (and did not verify more than seeing the "git show"
> output with "--color-moved") that other changes in this step are all
> pure moves?
Yup.
^ permalink raw reply
* Re: [PATCH 02/10] trailer: include "trailer" term in API functions
From: Junio C Hamano @ 2024-01-19 0:15 UTC (permalink / raw)
To: Linus Arver
Cc: Linus Arver via GitGitGadget, git, Emily Shaffer,
Christian Couder
In-Reply-To: <owlyv87qgozb.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Ah very true. I think I was thinking of the series as one atomic thing
> (all 10 patches or nothing), which is a bad habit I need to break. I'll
> reorder these two on the next reroll, because principles matter (and
> it's an easy mechanical change). Thanks.
Thanks. I wouldn't have minded if these renames and moves were done
in a single step, but because you already have them as separate two
steps, let's keep them separate.
^ permalink raw reply
* Re: [PATCH 02/10] trailer: include "trailer" term in API functions
From: Linus Arver @ 2024-01-19 0:12 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder
In-Reply-To: <xmqqfryup98u.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Linus Arver <linusa@google.com>
>>
>> These functions are exposed to clients and so they should include
>> "trailer" in their names for easier identification, just like all the
>> other functions already exposed by trailer.h.
>
> Oh, if you were to do this and code movement in two separate
> patches, doing the rename before the move in the previous step would
> have made much more sense. If we stopped after 01/10, the tree
> would have been a very sorry state. If this came first, even if we
> stop after these renaming of internal functions that are not extern,
> nobody wil be hurt by these new and improved names.
Ah very true. I think I was thinking of the series as one atomic thing
(all 10 patches or nothing), which is a bad habit I need to break. I'll
reorder these two on the next reroll, because principles matter (and
it's an easy mechanical change). Thanks.
^ permalink raw reply
* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Linus Arver @ 2024-01-18 23:59 UTC (permalink / raw)
To: Jeff King; +Cc: Taylor Blau, git, Junio C Hamano
In-Reply-To: <20240116151029.GC2119690@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Jan 15, 2024 at 02:31:12PM -0800, Linus Arver wrote:
>
>> > +static int pair_chunk_expect_fn(const unsigned char *chunk_start,
>> > + size_t chunk_size,
>> > + void *data)
>> > +{
>> > + struct pair_chunk_data *pcd = data;
>> > + if (chunk_size / pcd->record_size != pcd->record_nr)
>> > + return -1;
>> > + *pcd->p = chunk_start;
>> > + return 0;
>> > +}
>> > +
>>
>> I don't think this function should assume anything about the inputs
>> (chunk_size, record_size, nor record_nr). If we are asking the helper to
>> be the common tool for multiple locations, it should assume even less
>> about what these inputs could look like.
>>
>> For example, if record_size is 0 this will lead to a
>> divide-by-zero. Likewise, if record_nr is zero it doesn't make
>> sense to check if chunk_size fits into record_size * record_nr.
>
> I'm not sure that divide-by-zero is a big deal, because 0-length
> fixed-size records do not make any sense to ask about.
So why not make this function check for this? While it may be true that
0-length fixed-size records are impossible currently, nothing guarantees
they will always be that way all the time in the future.
> And even if
> somebody accidentally passed 0, even though it won't be caught by the
> compiler, it would barf on any input, so even rudimentary testing would
> catch it.
If somebody is accidentally passing an invalid value to a function, then
it feels right for that function to be able to handle it instead of
crashing (or doing any other undefined behavior) with division-by-zero.
Taking a step back though, maybe I am being overly defensive about
possible failure modes. I don't know the surrounding area well so I
might be overreacting.
> A zero record_nr is a perfectly reasonable thing to ask about. If you
> have a chunk file with zero entries for some entity, then we are
> checking that the chunk is the expected zero length as well.
Right.
>> And even if there are no (unexpected) zero-values involved, shouldn't we
>> also check for nonsensical comparisons, such as if chunk_size is smaller
>> than record_size?
>
> Aren't we checking that already? If chunk_size is less than record_size,
> then the division will result in "0". If the expected number of records
> is not also 0, then that's a bogus file.
I was thinking of an early return like
if (chunk_size < record_size)
return CHUNK_TOO_SMALL
before evaluating (chunk_size / pcd->record_size != pcd->record_nr).
You're correct that the division will result in "0" if chunk_size is
less than record_size. But I didn't like having the extra mental load
for reading and understanding the correctness of "if (chunk_size /
pcd->record_size != pcd->record_nr)" for that case. IOW, the more early
returns we have for known-bad cases, by the time we get to "if
(chunk_size / pcd->record_size != pcd->record_nr)" it would be that much
easier to understand that code.
> What we really care about here is that we won't walk off the end of the
> buffer while looking at N fixed-size records ...
Ah, I see. This sort of insight would be great to have as a comment in
the code.
> ... (in that sense, the "too
> big" test is overly careful, but it's easy to include as a sanity
> check).
OK.
>> So in summary there appear to be the following possibilities:
>>
>> CHUNK_MISSING
>> CHUNK_TOO_SMALL
>> CHUNK_OK
>> CHUNK_TOO_BIG_ALIGNED
>> CHUNK_TOO_BIG_MISALIGNED
>
> So yes, I agree all these cases exist. We detect them all except the
> "misaligned" case (which I think was discussed earlier in the thread as
> not worth caring too much about).
OK.
> But...
>
>> (on top of the cases where record_* inputs are zero).
>>
>> And it seems prudent to treat each of the not-OK cases differently
>> (including how we report error messages) once we know which category we
>> fall into.
>
> I'm not sure it is worth caring too much about the different cases. From
> the caller's perspective the end result is always to avoid using the
> chunk/file.
Ah OK. Then yes, it does seem like caring about the different cases is
too much from the callers' perspective.
But I do think that checking the different cases with early returns will
at least help readability (and as a bonus assure future Git developers
that divide-by-zero errors are impossible).
^ permalink raw reply
* Re: [PATCH 2/2] t0024: refactor to have single command per line
From: Junio C Hamano @ 2024-01-18 23:18 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <20240118215407.8609-2-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> Refactor t0024 to avoid having multiple chaining commands on a single
> line, according to current styling norms.
>
> e.g turn
> ( mkdir testdir && cd testdir && echo "in testdir" )
> into:
> mkdir testdir &&
> (
> cd testdir &&
> echo "in testdir"
> )
>
> This is also described in the Documentation/CodingGuidelines file.
Sure.
Subject: t0024: style fix
t0024 has multiple command invocations on a single line,
which goes against the style given by CodingGuidelines.
would be sufficient.
> - ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) &&
> + mkdir untarred &&
> + (
> + cd untarred &&
> + "$TAR" -xf ../test.tar
> + ) &&
I think we assume "$TAR" is modern enough to know about the "C"
option (see t/t5004-archive-corner-cases.sh), so
mkdir untarred &&
"$TAR" Cxf untarred test.tar
without even a subshell may be sufficient.
> @@ -30,7 +34,11 @@ test_expect_success UNZIP 'zip archive' '
>
> git archive --format=zip HEAD >test.zip &&
>
> - ( mkdir unzipped && cd unzipped && "$GIT_UNZIP" ../test.zip ) &&
> + mkdir unzipped &&
> + (
> + cd unzipped &&
> + "$GIT_UNZIP" ../test.zip
> + ) &&
I do not think we assume "$GIT_UNZIP" to always know about the
equivalent of "C" (is that "-d exdir"?), so what you wrote is the
best we can do.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] t0024: avoid losing exit status to pipes
From: Junio C Hamano @ 2024-01-18 23:04 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <20240118215407.8609-1-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> Replace pipe with redirection operator '>' to store the output
> to a temporary file after 'git archive' command since the pipe
> will swallow the command's exit code and a crash won't
> necessarily be noticed.
OK. I think this case what the patch does is the right thing. If
we were creating a huge tar archive and have the downstream only
consuming for a small disk footprint (e.g., "git archive | tar tf -"),
use of pipe (and loss of exit status) may be justified, but I do not
think that is the case here.
> Also refactor an existing use of '>' to avoid having a space after
> '>', according to Documentation/CodingGuidelines.
It may be just me, but I wouldn't call that "refactor", which we
often use to refer to changing the way an existing functional unit
works internally, by splitting the innard of a function into
separate helper functions, by replacing the open-coded duplicate
with calls to such helper functions, etc. Correcting the coding
style violation is merely a "fix".
Also fix an unwanted space after '>' redirection to match
the style in CodingGuidelines.
In any case, the patch text looks good.
Thanks.
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
> t/t0024-crlf-archive.sh | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
> index a34de56420..fa4da7c2b3 100755
> --- a/t/t0024-crlf-archive.sh
> +++ b/t/t0024-crlf-archive.sh
> @@ -9,7 +9,7 @@ test_expect_success setup '
>
> git config core.autocrlf true &&
>
> - printf "CRLF line ending\r\nAnd another\r\n" > sample &&
> + printf "CRLF line ending\r\nAnd another\r\n" >sample &&
> git add sample &&
>
> test_tick &&
> @@ -19,8 +19,8 @@ test_expect_success setup '
>
> test_expect_success 'tar archive' '
>
> - git archive --format=tar HEAD |
> - ( mkdir untarred && cd untarred && "$TAR" -xf - ) &&
> + git archive --format=tar HEAD >test.tar &&
> + ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) &&
>
> test_cmp sample untarred/sample
^ permalink raw reply
* Re: [PATCH 03/10] trailer: unify trailer formatting machinery
From: Junio C Hamano @ 2024-01-18 22:56 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder, Linus Arver
In-Reply-To: <d3326021fb6a63707e4ce56f166447143f4e55a2.1704869487.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> Currently have two functions for formatting trailers exposed in
> trailer.h:
>
> void format_trailers(FILE *outfile, struct list_head *head,
> const struct process_trailer_options *opts);
>
> void format_trailers_from_commit(struct strbuf *out, const char *msg,
> const struct process_trailer_options *opts);
>
> and previously these functions, although similar enough (even taking the
> same process_trailer_options struct pointer), did not build on each
> other.
>
> Make format_trailers_from_commit() rely on format_trailers(). Teach
> format_trailers() to process trailers with the additional
> process_trailer_options fields like opts->key_only which is only used by
> format_trailers_from_commit() and not builtin/interpret-trailers.c.
Yay. It feels a bit disappointing to see the diffstat and learn
that we are not deleting substantial number of lines.
> ---
> builtin/interpret-trailers.c | 5 +-
> pretty.c | 2 +-
> ref-filter.c | 2 +-
> trailer.c | 105 +++++++++++++++++++++++++++++------
> trailer.h | 21 +++----
> 5 files changed, 102 insertions(+), 33 deletions(-)
> diff --git a/pretty.c b/pretty.c
> index cf964b060cd..f0721a5214f 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1759,7 +1759,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
> goto trailer_out;
> }
> if (*arg == ')') {
> - format_trailers_from_commit(sb, msg + c->subject_off, &opts);
> + format_trailers_from_commit(msg + c->subject_off, &opts, sb);
I am curious (read: no objection---merely wondering if there is a
guiding principle behind the choice of the new order) why this new
parameter ordering. I suspect it was originally written with a
strbuf-centric worldview and having sb at the beginning may have
made sense, but if we are moving them around, wouldn't it make more
sense to have &opts as the first parameter, as this is primarily a
"trailers" function? Unsure until I read through to the end, but
that is my gut reaction.
> static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
> {
> struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
> @@ -984,6 +971,78 @@ static void unfold_value(struct strbuf *val)
> strbuf_release(&out);
> }
>
> +void format_trailers(struct list_head *head,
> + const struct process_trailer_options *opts,
> + struct strbuf *out)
> +{
> + struct list_head *pos;
> + struct trailer_item *item;
> + int need_separator = 0;
> +
> + list_for_each(pos, head) {
> + item = list_entry(pos, struct trailer_item, list);
> + if (item->token) {
> + char c;
> + ...
> + if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
> + if (opts->unfold)
> + unfold_value(&val);
> +
> + if (opts->separator && need_separator)
> + strbuf_addbuf(out, opts->separator);
> + if (!opts->value_only)
> + strbuf_addbuf(out, &tok);
> + if (!opts->key_only && !opts->value_only) {
> + if (opts->key_value_separator)
> + strbuf_addbuf(out, opts->key_value_separator);
> + else {
> + c = last_non_space_char(tok.buf);
> + if (c) {
> + if (!strchr(separators, c))
> + strbuf_addf(out, "%c ", separators[0]);
> + }
> + }
That's an overly deep nesting. I wonder if a small file-scope
helper function is in order?
static add_separator(struct process_trailer_options *opts,
const char *token
struct strbuf *out)
{
if (opts->key_value_separator)
strbuf_addbuf(out, opts->key_value_separator);
else
strbuf_addstr(out, ": ");
}
Or perhaps inside the context of the loop to go over the list of
trailer items, one iteration of the list_for_each() loop can become
one call to a small helper function format_one_trailer() and that
may make the result easier to follow?
In any case, I didn't see anything glaringly wrong so far in this
series. Let me keep reading.
Thanks.
^ permalink raw reply
* Re: [PATCH 02/10] trailer: include "trailer" term in API functions
From: Junio C Hamano @ 2024-01-18 22:28 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder, Linus Arver
In-Reply-To: <5f64718abfc2e61b4e259de700c137bc817fbb1c.1704869487.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> These functions are exposed to clients and so they should include
> "trailer" in their names for easier identification, just like all the
> other functions already exposed by trailer.h.
Oh, if you were to do this and code movement in two separate
patches, doing the rename before the move in the previous step would
have made much more sense. If we stopped after 01/10, the tree
would have been a very sorry state. If this came first, even if we
stop after these renaming of internal functions that are not extern,
nobody wil be hurt by these new and improved names.
^ permalink raw reply
* Re: [PATCH 01/10] trailer: move process_trailers() to interpret-trailers.c
From: Junio C Hamano @ 2024-01-18 22:26 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder, Linus Arver
In-Reply-To: <2dc3279b37f3aa81ba20e3dd08b029ca655ac3d8.1704869487.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/trailer.h b/trailer.h
> index 1644cd05f60..b3e4a5e127d 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -81,15 +81,29 @@ struct process_trailer_options {
>
> #define PROCESS_TRAILER_OPTIONS_INIT {0}
>
> -void process_trailers(const char *file,
> - const struct process_trailer_options *opts,
> - struct list_head *new_trailer_head);
> +void parse_trailers_from_config(struct list_head *config_head);
> +
> +void parse_trailers_from_command_line_args(struct list_head *arg_head,
> + struct list_head *new_trailer_head);
> +
> +void process_trailers_lists(struct list_head *head,
> + struct list_head *arg_head);
> +
> +void parse_trailers(struct trailer_info *info,
> + const char *str,
> + struct list_head *head,
> + const struct process_trailer_options *opts);
OK.
> +void ensure_configured(void);
> +void print_all(FILE *outfile, struct list_head *head,
> + const struct process_trailer_options *opts);
> +void free_all(struct list_head *head);
All of these names are way overly generic to live in the global
namespace, no? Granted, they may only be visible to folks who
include <trailer.h>, but sooner or later other subsystems would find
need to provide library-ish functions that allows their callers to
do these things to their subsystem: make sure the subsystem has been
initialized, print info on all "things" on a list that the subsystem
works on, and free all "things" on such a list.
It is sad to have a function that takes "struct list_head *" as its
parameter without having any comment on what the elements on that
list are about. free_trailer_items() might be a more appropriate
name (or free_all_trailer_items(), if we foresee a need to just free
the first item on the list and give such a helper
free_trailer_item(), in which case, the distinction between "item"
vs "items" might become too subtle.
I am assuming (and did not verify more than seeing the "git show"
output with "--color-moved") that other changes in this step are all
pure moves? If so, other than need to be careful about naming when
making things from static to extern pointed out above, this step
looks OK to me.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper
From: Linus Arver @ 2024-01-18 22:26 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <CANYiYbFOa-E8Pivhgn_nmy982fn7VPtb803bewnC_UV7qY3xcw@mail.gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
>> > Remove the restriction in the "connect_helper()" function and give the
>> > function "process_connect_service()" the opportunity to establish a
>> > connection using ".connect" or ".stateless_connect" for protocol v2. So
>> > we can connect with a stateless-rpc and do something useful. E.g., in a
>> > later commit, implements remote archive for a repository over HTTP
>> > protocol.
>> >
>> > Helped-by: Junio C Hamano <gitster@pobox.com>
>> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>> > ---
>> > transport-helper.c | 2 --
>> > 1 file changed, 2 deletions(-)
>> >
>> > diff --git a/transport-helper.c b/transport-helper.c
>> > index 49811ef176..2e127d24a5 100644
>> > --- a/transport-helper.c
>> > +++ b/transport-helper.c
>> > @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
>> >
>> > /* Get_helper so connect is inited. */
>> > get_helper(transport);
>> > - if (!data->connect)
>> > - die(_("operation not supported by protocol"));
>>
>> Should we still terminate early here if both data->connect and
>> data->stateless_connect are not truthy? It would save a few CPU cycles,
>> but even better, remain true to the the original intent of the code.
>> Maybe there was a really good reason to terminate early here that we're
>> not aware of?
>>
>
> It's not necessary to check data->connect here, because it will
> terminate if fail to connect by calling the function
> "process_connect_service()".
In the process_connect_service() we have
if (data->connect) {
...
} else if (data->stateless_connect && ...) {
...
}
strbuf_release(&cmdbuf);
return ret;
and so if both data->connect and data->stateless_connect are false, that
function could silently do nothing. IOW that function expects the
connection type to be guaranteed to be set, so it makes sense to check
for the correctness of this in the connect_helper().
>> But also, what about the case where both are enabled? Should we print an
>> error message? (Maybe this concern is outside the scope of this series?)
>
> In the function "process_connect_service()", we can see that "connect"
> has a higher priority than "stateless-connect".
What I mean is, does it make sense for connect_helper() to recognize
invalid or possibly buggy states? IOW, is having both data->connect and
data->stateless_connect enabled a bug? If we only ever set one or the
other (we treat them as mutually exclusive) elsewhere in the codebase,
and if we are doing the sort of "correctness" check in the
connect_helper(), then it makes sense to detect that both are set and
print an error or warning (as a programmer bug).
^ permalink raw reply
* [PATCH v3] merge-ll: expose revision names to custom drivers
From: Antonin Delpeuch via GitGitGadget @ 2024-01-18 22:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Antonin Delpeuch, Antonin Delpeuch
In-Reply-To: <pull.1648.v2.git.git.1705592581272.gitgitgadget@gmail.com>
From: Antonin Delpeuch <antonin@delpeuch.eu>
Custom merge drivers need access to the names of the revisions they
are working on, so that the merge conflict markers they introduce
can refer to those revisions. The placeholders '%S', '%X' and '%Y'
are introduced to this end.
Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>
---
merge-ll: expose revision names to custom drivers
Changes since v2:
* change the documentation to use "common ancestor" rather than "merge
ancestor"
* fix indentation issue
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1648%2Fwetneb%2Fmerge_driver_pathnames-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1648/wetneb/merge_driver_pathnames-v3
Pull-Request: https://github.com/git/git/pull/1648
Range-diff vs v2:
1: 6dec70529c0 ! 1: aebd26711fe merge-ll: expose revision names to custom drivers
@@ Commit message
Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>
## Documentation/gitattributes.txt ##
-@@ Documentation/gitattributes.txt: command to run to merge ancestor's version (`%O`), current
+@@ Documentation/gitattributes.txt: The `merge.*.name` variable gives the driver a human-readable
+ name.
+
+ The `merge.*.driver` variable's value is used to construct a
+-command to run to merge ancestor's version (`%O`), current
++command to run to common ancestor's version (`%O`), current
version (`%A`) and the other branches' version (`%B`). These
three tokens are replaced with the names of temporary files that
hold the contents of these versions when the command line is
@@ Documentation/gitattributes.txt: When left unspecified, the driver itself is use
-will be stored via placeholder `%P`.
-
+will be stored via placeholder `%P`. Additionally, the names of the
-+merge ancestor revision (`%S`), of the current revision (`%X`) and
++common ancestor revision (`%S`), of the current revision (`%X`) and
+of the other branch (`%Y`) can also be supplied. Those are short
+revision names, optionally joined with the paths of the file in each
+revision. Those paths are only present if they differ and are separated
@@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive
else if (skip_prefix(format, "P", &format))
sq_quote_buf(&cmd, path);
+ else if (skip_prefix(format, "S", &format))
-+ sq_quote_buf(&cmd, orig_name);
++ sq_quote_buf(&cmd, orig_name);
+ else if (skip_prefix(format, "X", &format))
+ sq_quote_buf(&cmd, name1);
+ else if (skip_prefix(format, "Y", &format))
Documentation/gitattributes.txt | 12 ++++++++----
merge-ll.c | 17 ++++++++++++++---
t/t6406-merge-attr.sh | 16 +++++++++++-----
3 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 201bdf5edbd..86a0946bb9e 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -1137,11 +1137,11 @@ The `merge.*.name` variable gives the driver a human-readable
name.
The `merge.*.driver` variable's value is used to construct a
-command to run to merge ancestor's version (`%O`), current
+command to run to common ancestor's version (`%O`), current
version (`%A`) and the other branches' version (`%B`). These
three tokens are replaced with the names of temporary files that
hold the contents of these versions when the command line is
-built. Additionally, %L will be replaced with the conflict marker
+built. Additionally, `%L` will be replaced with the conflict marker
size (see below).
The merge driver is expected to leave the result of the merge in
@@ -1159,8 +1159,12 @@ When left unspecified, the driver itself is used for both
internal merge and the final merge.
The merge driver can learn the pathname in which the merged result
-will be stored via placeholder `%P`.
-
+will be stored via placeholder `%P`. Additionally, the names of the
+common ancestor revision (`%S`), of the current revision (`%X`) and
+of the other branch (`%Y`) can also be supplied. Those are short
+revision names, optionally joined with the paths of the file in each
+revision. Those paths are only present if they differ and are separated
+from the revision by a colon.
`conflict-marker-size`
^^^^^^^^^^^^^^^^^^^^^^
diff --git a/merge-ll.c b/merge-ll.c
index 1df58ebaac0..13e0713fe82 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -185,9 +185,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
mmbuffer_t *result,
const char *path,
- mmfile_t *orig, const char *orig_name UNUSED,
- mmfile_t *src1, const char *name1 UNUSED,
- mmfile_t *src2, const char *name2 UNUSED,
+ mmfile_t *orig, const char *orig_name,
+ mmfile_t *src1, const char *name1,
+ mmfile_t *src2, const char *name2,
const struct ll_merge_options *opts,
int marker_size)
{
@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
strbuf_addf(&cmd, "%d", marker_size);
else if (skip_prefix(format, "P", &format))
sq_quote_buf(&cmd, path);
+ else if (skip_prefix(format, "S", &format))
+ sq_quote_buf(&cmd, orig_name);
+ else if (skip_prefix(format, "X", &format))
+ sq_quote_buf(&cmd, name1);
+ else if (skip_prefix(format, "Y", &format))
+ sq_quote_buf(&cmd, name2);
else
strbuf_addch(&cmd, '%');
}
@@ -315,7 +321,12 @@ static int read_merge_config(const char *var, const char *value,
* %B - temporary file name for the other branches' version.
* %L - conflict marker length
* %P - the original path (safely quoted for the shell)
+ * %S - the revision for the merge base
+ * %X - the revision for our version
+ * %Y - the revision for their version
*
+ * If the file is not named indentically in all versions, then each
+ * revision is joined with the corresponding path, separated by a colon.
* The external merge driver should write the results in the
* file named by %A, and signal that it has done with zero exit
* status.
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 72f8c1722ff..156a1efacfe 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -42,11 +42,15 @@ test_expect_success setup '
#!/bin/sh
orig="$1" ours="$2" theirs="$3" exit="$4" path=$5
+ orig_name="$6" our_name="$7" their_name="$8"
(
echo "orig is $orig"
echo "ours is $ours"
echo "theirs is $theirs"
echo "path is $path"
+ echo "orig_name is $orig_name"
+ echo "our_name is $our_name"
+ echo "their_name is $their_name"
echo "=== orig ==="
cat "$orig"
echo "=== ours ==="
@@ -121,7 +125,7 @@ test_expect_success 'custom merge backend' '
git reset --hard anchor &&
git config --replace-all \
- merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+ merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
git config --replace-all \
merge.custom.name "custom merge driver for testing" &&
@@ -132,7 +136,8 @@ test_expect_success 'custom merge backend' '
o=$(git unpack-file main^:text) &&
a=$(git unpack-file side^:text) &&
b=$(git unpack-file main:text) &&
- sh -c "./custom-merge $o $a $b 0 text" &&
+ base_revid=$(git rev-parse --short main^) &&
+ sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
sed -e 1,3d $a >check-2 &&
cmp check-1 check-2 &&
rm -f $o $a $b
@@ -142,7 +147,7 @@ test_expect_success 'custom merge backend' '
git reset --hard anchor &&
git config --replace-all \
- merge.custom.driver "./custom-merge %O %A %B 1 %P" &&
+ merge.custom.driver "./custom-merge %O %A %B 1 %P %S %X %Y" &&
git config --replace-all \
merge.custom.name "custom merge driver for testing" &&
@@ -159,7 +164,8 @@ test_expect_success 'custom merge backend' '
o=$(git unpack-file main^:text) &&
a=$(git unpack-file anchor:text) &&
b=$(git unpack-file main:text) &&
- sh -c "./custom-merge $o $a $b 0 text" &&
+ base_revid=$(git rev-parse --short main^) &&
+ sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
sed -e 1,3d $a >check-2 &&
cmp check-1 check-2 &&
sed -e 1,3d -e 4q $a >check-3 &&
@@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
git reset --hard anchor &&
git config --replace-all \
- merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+ merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
git config --replace-all \
merge.custom.name "custom merge driver for testing" &&
base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/2] t0024: refactor to have single command per line
From: Ghanshyam Thakkar @ 2024-01-18 21:53 UTC (permalink / raw)
To: git; +Cc: Ghanshyam Thakkar
In-Reply-To: <20240118215407.8609-1-shyamthakkar001@gmail.com>
Refactor t0024 to avoid having multiple chaining commands on a single
line, according to current styling norms.
e.g turn
( mkdir testdir && cd testdir && echo "in testdir" )
into:
mkdir testdir &&
(
cd testdir &&
echo "in testdir"
)
This is also described in the Documentation/CodingGuidelines file.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t0024-crlf-archive.sh | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index fa4da7c2b3..ff4efeaaee 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -20,7 +20,11 @@ test_expect_success setup '
test_expect_success 'tar archive' '
git archive --format=tar HEAD >test.tar &&
- ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) &&
+ mkdir untarred &&
+ (
+ cd untarred &&
+ "$TAR" -xf ../test.tar
+ ) &&
test_cmp sample untarred/sample
@@ -30,7 +34,11 @@ test_expect_success UNZIP 'zip archive' '
git archive --format=zip HEAD >test.zip &&
- ( mkdir unzipped && cd unzipped && "$GIT_UNZIP" ../test.zip ) &&
+ mkdir unzipped &&
+ (
+ cd unzipped &&
+ "$GIT_UNZIP" ../test.zip
+ ) &&
test_cmp sample unzipped/sample
--
2.43.0
^ permalink raw reply related
* [PATCH 1/2] t0024: avoid losing exit status to pipes
From: Ghanshyam Thakkar @ 2024-01-18 21:53 UTC (permalink / raw)
To: git; +Cc: Ghanshyam Thakkar
Replace pipe with redirection operator '>' to store the output
to a temporary file after 'git archive' command since the pipe
will swallow the command's exit code and a crash won't
necessarily be noticed.
Also refactor an existing use of '>' to avoid having a space after
'>', according to Documentation/CodingGuidelines.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t0024-crlf-archive.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index a34de56420..fa4da7c2b3 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -9,7 +9,7 @@ test_expect_success setup '
git config core.autocrlf true &&
- printf "CRLF line ending\r\nAnd another\r\n" > sample &&
+ printf "CRLF line ending\r\nAnd another\r\n" >sample &&
git add sample &&
test_tick &&
@@ -19,8 +19,8 @@ test_expect_success setup '
test_expect_success 'tar archive' '
- git archive --format=tar HEAD |
- ( mkdir untarred && cd untarred && "$TAR" -xf - ) &&
+ git archive --format=tar HEAD >test.tar &&
+ ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) &&
test_cmp sample untarred/sample
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] precious-files.txt: new document proposing new precious file type
From: Sebastian Thiel @ 2024-01-18 21:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren, Elijah Newren via GitGitGadget, git, Josh Triplett,
Phillip Wood
In-Reply-To: <xmqqwms6qwr3.fsf@gitster.g>
Thanks so much for the analysis, as seeing the problem of choosing
a syntax from the perspective of its effects when using common commands
like "git add" and "git clean -f" seems very promising!
When thinking about "git add ." vs "git clean -f" one difference comes to
mind: "git clean -f" is much less desirable it's fatal. "git add ." on the
other hand leaves room for correction, even when used with `git commit -a"
(and with the exception of "git commit -am 'too late'").
From that point of view I'd naturally prefer the "$.config" syntax as it
will turn precious files into untracked ones for current Git.
> * Which one between "'git add .' adds '.config' that users did not
> want to add" and "'git clean -f' removes '.config' together with
> other files" a larger problem to the users, who participate in a
> project that already decided to use the new .gitignore feature to
> mark ".config" as "precious", of older versions of Git that
> predate "precious"?
>
If the user should have a choice, than both syntaxes could also be allowed
to let them choose what to optimise for.
Doing so might be less relevant in the `.config` case, but most relevant
for ignored files like ".env" or ".env.secret" which under no circumstances
must be tracked.
> * What are projects doing to paths that they want to make
> "precious" with the current system? Do they leave them out of
> ".gitignore" and have them subject to accidental "git add ." to
> protect them from "git clean -f"? Or do they list them in
> ".gitignore" to prevent "git add ." from touching, but leave them
> susceptible to accidental removal by "git clean -f"?
I did hear that some projects use make files with specifically configured
"git clean" invocations to specifically "--exclude" precious files.
Thus far I didn't encounter one that use such a technique to prevent
"git add" from tracking too much though.
To my mind, in order to support projects with both ".config" and
".env.secret" they would have to be given a choice of which syntax
to use, e.g.
# This file shouldn't accidentally be deleted by `git clean`
$.config
# These files should never be accidentally tracked
#(keep)
.env*
On 18 Jan 2024, at 20:14, Junio C Hamano wrote:
> Sebastian Thiel <sebastian.thiel@icloud.com> writes:
>
>> #(keep)
>> .config
>>
>> As a side-effect of the syntax, it's obvious this is an 'upgrade', with
>> perfect backwards compatibility as old git does the same as always.
>
> Yes but ...
>
> The point Elijah makes is worth considering. To existing versions
> of git, having this entry for ".config" means that it is ignored
> (i.e. "git add ." will not include it), but expendable (i.e. "git
> clean" considers ".config" as a candidate for removal; "git checkout
> other", if the "other" branch has it as a tracked path, will clobber
> it). Compared to the case where ".config" is not mentioned in
> ".gitignore", where it may be added by use of "git add .", it won't
> be clobbered by "git clean".
>
> So this syntax having "perfect backward compatibility" is not quite
> true. It does have downsides when used by existing versions of Git.
>
> If we use Elijah's syntax to say
>
> $.config
>
> then the entry to existing versions of git is a no-op wrt a file
> named ".config". It simply does not match the pattern, so an
> accidental "git add ." *will* add ".config" to the index, while "git
> clean" may not touch it, simply because it is treated as "untracked
> and precious". In other words, its downside is the same as not
> marking the path ".config" in any way in ".gitignore", as far as
> existing versions of Git are concerned.
>
> We of course discount the possibility that people keep a file whose
> name literally is dollar-dot followed by "config" and older versions
> of Git would start treating them as ignored-and-expendable. While
> it *is* an additional downside compared to Phillip's "#(keep)"
> approach, I do not think that particular downside is worth worrying
> about. Yet another downside compared to Phillip's is that it is
> less extensible. Over the years, however, the ignored-but-precious
> is the only one we heard from users that lack of which is hurting
> them, so lack of extensibility may not be too huge a deal.
>
> For projects that are currently listing these files in ".gitignore"
> as "ignored-and-expendable" already and want to categorize them as
> "ignored-and-precious" by changing ".config" to "$.config" (or
> adding "#(keep)" comment before the existing entry), the
> pros-and-cons equation may differ. Their current participants are
> protected from accidentally adding them with "git add ." but risking
> to lose them with "git clean -f". They may even be trained to be
> careful to see "git clean -n" output before actually running the
> command with "-f". Now, if their project ships a new version of
> ".gitignore" that marks these paths as "ignored-and-precious", both
> approaches will have intended effect to participants who upgraded to
> the version of Git.
>
> To participants using the current version of Git:
>
> * Phillip's approach to add "#(keep)" will not change anything.
> They will be protected from accidental "git add ." as before, and
> they will still have to be careful about "git clean -f".
>
> * Elijah's approach to rewrite existing'.config' to '$.config',
> however, will stop protecting them from "git add .", even though
> it will start protecting them from "git clean -f".
>
> The devil you already know may be the lessor of two evils in such a
> situation.
>
> So, all it boils down to is these two questions.
>
> * Which one between "'git add .' adds '.config' that users did not
> want to add" and "'git clean -f' removes '.config' together with
> other files" a larger problem to the users, who participate in a
> project that already decided to use the new .gitignore feature to
> mark ".config" as "precious", of older versions of Git that
> predate "precious"?
>
> * What are projects doing to paths that they want to make
> "precious" with the current system? Do they leave them out of
> ".gitignore" and have them subject to accidental "git add ." to
> protect them from "git clean -f"? Or do they list them in
> ".gitignore" to prevent "git add ." from touching, but leave them
> susceptible to accidental removal by "git clean -f"?
>
> Thanks.
^ permalink raw reply
* Re: [PATCH v2] merge-ll: expose revision names to custom drivers
From: Antonin Delpeuch @ 2024-01-18 20:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1qaeqtw7.fsf@gitster.g>
Hi Junio,
Thanks a lot for your review! (and many apologies for the double sending
as HTML…)
On 18/01/2024 21:16, Junio C Hamano wrote:
> Or you could fix %O's description "while at it" and use the right
> term from the get-go for %S.
Agreed, I'll do that, it also feels more fitting to me.
> I see some funny indentation for "S" here.
Oops, sorry about that.
>> @@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
>>
>> git reset --hard anchor &&
>> git config --replace-all \
>> - merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
>> + merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
>> git config --replace-all \
>> merge.custom.name "custom merge driver for testing" &&
> ;-)
>
> This one is expected to die and not produce meaningful output;
> I was wondering why this does not need to make corresponding changes
> to the expected output pattern like the earlier tests.
As far as I can tell, this test does not compare the result of the merge
to the expected merge driver output, because that output is expected to
be disregarded by git given that the merge driver died (see the last two
lines). So it seems normal to me that we don't need to adapt the
expected output: the repository files are still left unchanged.
I'll submit a new version of the patch with the two changes above.
Antonin
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Dragan Simic @ 2024-01-18 20:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Rubén Justo, Git List
In-Reply-To: <xmqqwms6pf72.fsf@gitster.g>
On 2024-01-18 21:19, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>> On 2024-01-18 19:26, Junio C Hamano wrote:
>>> Dragan Simic <dsimic@manjaro.org> writes:
>>>
>>>> Perhaps having both options available could be a good thing.
>>>> Though,
>>>> adding quite a few knobs may end up confusing the users, so we
>>>> should
>>>> make sure to document it well.
>>>
>>> I agree there is a need for documentation, but we are not increasing
>>> the number of knobs, are we?
>>
>> Perhaps there would be one more knob if we end up deciding to
>> implement
>> support for both approaches, as previously discussed.
>
> But that would be just one knob (which I personally do not quite see
> the need for), not "quite a few knobs" as you said, no?
I'm sorry for being imprecise. Regarding the need for that additional
"global" knob, I think it can't hurt. Some people might even find it
quite useful, and the good thing is that it wouldn't make the internal
logic significantly more complicated.
^ permalink raw reply
* Re: [PATCH v2 2/4] test-submodule: remove command line handling for check-name
From: Junio C Hamano @ 2024-01-18 20:44 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget
Cc: git, Patrick Steinhardt, Jeff King, Victoria Dye
In-Reply-To: <14e8834c38bcddc21856772b09f6fa77fa924b48.1705542918.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Victoria Dye <vdye@github.com>
>
> The 'check-name' subcommand to 'test-tool submodule' is documented as being
> able to take a command line argument '<name>'. However, this does not work -
> and has never worked - because 'argc > 0' triggers the usage message in
> 'cmd__submodule_check_name()'. To simplify the helper and avoid future
> confusion around proper use of the subcommand, remove any references to
> command line arguments for 'check-name' in usage strings and handling in
> 'check_name()'.
>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> t/helper/test-submodule.c | 29 +++++++++--------------------
> 1 file changed, 9 insertions(+), 20 deletions(-)
Excellent, both of you.
^ permalink raw reply
* [PATCH v3 5/5] completion: git-bisect view recognized but not completed
From: Britton Leo Kerin @ 2024-01-18 20:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240118204323.1113859-1-britton.kerin@gmail.com>
This allows the git-log options to be completed but avoids completion
ambiguity between visualize and view.
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ad80df6630..87cf7b2561 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1591,13 +1591,22 @@ _git_bisect ()
term_good=`__git bisect terms --term-good`
fi
- local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+ # We will complete any custom terms, but still always complete the
+ # more usual bad/new/good/old because git bisect gives a good error
+ # message if these are given when not in use and that's better than
+ # silent refusal to complete if the user is confused.
+ #
+ # We want to recognize 'view' but not complete it, because it overlaps
+ # with 'visualize' too much and is just an alias for it.
+ #
+ local completable_subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+ local all_subcommands="$completable_subcommands view"
- local subcommand="$(__git_find_on_cmdline "$subcommands")"
+ local subcommand="$(__git_find_on_cmdline "$all_subcommands")"
if [ -z "$subcommand" ]; then
if [ -f "$__git_repo_path"/BISECT_START ]; then
- __gitcomp "$subcommands"
+ __gitcomp "$completable_subcommands"
else
__gitcomp "replay start"
fi
@@ -1615,7 +1624,7 @@ _git_bisect ()
;;
esac
;;
- visualize)
+ visualize|view)
case "$cur" in
-*)
__git_complete_log_opts
--
2.43.0
^ permalink raw reply related
* [PATCH v3 4/5] completion: custom git-bisect terms
From: Britton Leo Kerin @ 2024-01-18 20:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240118204323.1113859-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 63ca8082a4..ad80df6630 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1583,10 +1583,19 @@ _git_bisect ()
{
__git_has_doubledash && return
- local subcommands="start bad new good old terms skip reset visualize replay log run help"
+ __git_find_repo_path
+
+ local term_bad term_good
+ if [ -f "$__git_repo_path"/BISECT_START ]; then
+ term_bad=`__git bisect terms --term-bad`
+ term_good=`__git bisect terms --term-good`
+ fi
+
+ local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+
local subcommand="$(__git_find_on_cmdline "$subcommands")"
+
if [ -z "$subcommand" ]; then
- __git_find_repo_path
if [ -f "$__git_repo_path"/BISECT_START ]; then
__gitcomp "$subcommands"
else
@@ -1619,7 +1628,7 @@ _git_bisect ()
esac
case "$subcommand" in
- bad|new|good|old|reset|skip|start)
+ bad|new|"$term_bad"|good|old|"$term_good"|reset|skip|start)
__git_complete_refs
;;
*)
--
2.43.0
^ permalink raw reply related
* [PATCH v3 3/5] completion: move to maintain define-before-use
From: Britton Leo Kerin @ 2024-01-18 20:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240118204323.1113859-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 269 ++++++++++++-------------
1 file changed, 134 insertions(+), 135 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c16aded36c..63ca8082a4 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1445,6 +1445,140 @@ _git_archive ()
__git_complete_file
}
+# Options that go well for log, shortlog and gitk
+__git_log_common_options="
+ --not --all
+ --branches --tags --remotes
+ --first-parent --merges --no-merges
+ --max-count=
+ --max-age= --since= --after=
+ --min-age= --until= --before=
+ --min-parents= --max-parents=
+ --no-min-parents --no-max-parents
+"
+# Options that go well for log and gitk (not shortlog)
+__git_log_gitk_options="
+ --dense --sparse --full-history
+ --simplify-merges --simplify-by-decoration
+ --left-right --notes --no-notes
+"
+# Options that go well for log and shortlog (not gitk)
+__git_log_shortlog_options="
+ --author= --committer= --grep=
+ --all-match --invert-grep
+"
+# Options accepted by log and show
+__git_log_show_options="
+ --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
+"
+
+__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
+
+__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
+__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
+
+# Check for only porcelain (i.e. not git-rev-list) option (not argument)
+# and selected option argument completions for git-log options and if any
+# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
+# and will be empty on return if no candidates are found.
+__git_complete_log_opts ()
+{
+ [ -z "$COMPREPLY" ] || return 1 # Precondition
+
+ local merge=""
+ if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
+ merge="--merge"
+ fi
+ case "$prev,$cur" in
+ -L,:*:*)
+ return # fall back to Bash filename completion
+ ;;
+ -L,:*)
+ __git_complete_symbol --cur="${cur#:}" --sfx=":"
+ return
+ ;;
+ -G,*|-S,*)
+ __git_complete_symbol
+ return
+ ;;
+ esac
+ case "$cur" in
+ --pretty=*|--format=*)
+ __gitcomp "$__git_log_pretty_formats $(__git_pretty_aliases)
+ " "" "${cur#*=}"
+ return
+ ;;
+ --date=*)
+ __gitcomp "$__git_log_date_formats" "" "${cur##--date=}"
+ return
+ ;;
+ --decorate=*)
+ __gitcomp "full short no" "" "${cur##--decorate=}"
+ return
+ ;;
+ --diff-algorithm=*)
+ __gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
+ return
+ ;;
+ --submodule=*)
+ __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
+ return
+ ;;
+ --ws-error-highlight=*)
+ __gitcomp "$__git_ws_error_highlight_opts" "" "${cur##--ws-error-highlight=}"
+ return
+ ;;
+ --no-walk=*)
+ __gitcomp "sorted unsorted" "" "${cur##--no-walk=}"
+ return
+ ;;
+ --diff-merges=*)
+ __gitcomp "$__git_diff_merges_opts" "" "${cur##--diff-merges=}"
+ return
+ ;;
+ --*)
+ __gitcomp "
+ $__git_log_common_options
+ $__git_log_shortlog_options
+ $__git_log_gitk_options
+ $__git_log_show_options
+ --root --topo-order --date-order --reverse
+ --follow --full-diff
+ --abbrev-commit --no-abbrev-commit --abbrev=
+ --relative-date --date=
+ --pretty= --format= --oneline
+ --show-signature
+ --cherry-mark
+ --cherry-pick
+ --graph
+ --decorate --decorate= --no-decorate
+ --walk-reflogs
+ --no-walk --no-walk= --do-walk
+ --parents --children
+ --expand-tabs --expand-tabs= --no-expand-tabs
+ $merge
+ $__git_diff_common_options
+ "
+ return
+ ;;
+ -L:*:*)
+ return # fall back to Bash filename completion
+ ;;
+ -L:*)
+ __git_complete_symbol --cur="${cur#-L:}" --sfx=":"
+ return
+ ;;
+ -G*)
+ __git_complete_symbol --pfx="-G" --cur="${cur#-G}"
+ return
+ ;;
+ -S*)
+ __git_complete_symbol --pfx="-S" --cur="${cur#-S}"
+ return
+ ;;
+ esac
+}
+
_git_bisect ()
{
__git_has_doubledash && return
@@ -2052,141 +2186,6 @@ _git_ls_tree ()
__git_complete_file
}
-# Options that go well for log, shortlog and gitk
-__git_log_common_options="
- --not --all
- --branches --tags --remotes
- --first-parent --merges --no-merges
- --max-count=
- --max-age= --since= --after=
- --min-age= --until= --before=
- --min-parents= --max-parents=
- --no-min-parents --no-max-parents
-"
-# Options that go well for log and gitk (not shortlog)
-__git_log_gitk_options="
- --dense --sparse --full-history
- --simplify-merges --simplify-by-decoration
- --left-right --notes --no-notes
-"
-# Options that go well for log and shortlog (not gitk)
-__git_log_shortlog_options="
- --author= --committer= --grep=
- --all-match --invert-grep
-"
-# Options accepted by log and show
-__git_log_show_options="
- --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
-"
-
-__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
-
-__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
-__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-
-
-# Check for only porcelain (i.e. not git-rev-list) option (not argument)
-# and selected option argument completions for git-log options and if any
-# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
-# and will be empty on return if no candidates are found.
-__git_complete_log_opts ()
-{
- [ -z "$COMPREPLY" ] || return 1 # Precondition
-
- local merge=""
- if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
- merge="--merge"
- fi
- case "$prev,$cur" in
- -L,:*:*)
- return # fall back to Bash filename completion
- ;;
- -L,:*)
- __git_complete_symbol --cur="${cur#:}" --sfx=":"
- return
- ;;
- -G,*|-S,*)
- __git_complete_symbol
- return
- ;;
- esac
- case "$cur" in
- --pretty=*|--format=*)
- __gitcomp "$__git_log_pretty_formats $(__git_pretty_aliases)
- " "" "${cur#*=}"
- return
- ;;
- --date=*)
- __gitcomp "$__git_log_date_formats" "" "${cur##--date=}"
- return
- ;;
- --decorate=*)
- __gitcomp "full short no" "" "${cur##--decorate=}"
- return
- ;;
- --diff-algorithm=*)
- __gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
- return
- ;;
- --submodule=*)
- __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
- return
- ;;
- --ws-error-highlight=*)
- __gitcomp "$__git_ws_error_highlight_opts" "" "${cur##--ws-error-highlight=}"
- return
- ;;
- --no-walk=*)
- __gitcomp "sorted unsorted" "" "${cur##--no-walk=}"
- return
- ;;
- --diff-merges=*)
- __gitcomp "$__git_diff_merges_opts" "" "${cur##--diff-merges=}"
- return
- ;;
- --*)
- __gitcomp "
- $__git_log_common_options
- $__git_log_shortlog_options
- $__git_log_gitk_options
- $__git_log_show_options
- --root --topo-order --date-order --reverse
- --follow --full-diff
- --abbrev-commit --no-abbrev-commit --abbrev=
- --relative-date --date=
- --pretty= --format= --oneline
- --show-signature
- --cherry-mark
- --cherry-pick
- --graph
- --decorate --decorate= --no-decorate
- --walk-reflogs
- --no-walk --no-walk= --do-walk
- --parents --children
- --expand-tabs --expand-tabs= --no-expand-tabs
- $merge
- $__git_diff_common_options
- "
- return
- ;;
- -L:*:*)
- return # fall back to Bash filename completion
- ;;
- -L:*)
- __git_complete_symbol --cur="${cur#-L:}" --sfx=":"
- return
- ;;
- -G*)
- __git_complete_symbol --pfx="-G" --cur="${cur#-G}"
- return
- ;;
- -S*)
- __git_complete_symbol --pfx="-S" --cur="${cur#-S}"
- return
- ;;
- esac
-}
-
_git_log ()
{
__git_has_doubledash && return
--
2.43.0
^ permalink raw reply related
* [PATCH v3 2/5] completion: git-log opts to bisect visualize
From: Britton Leo Kerin @ 2024-01-18 20:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240118204323.1113859-1-britton.kerin@gmail.com>
To do this the majority of _git_log has been factored out into the new
__git_complete_log_opts. This is needed because the visualize command
accepts git-log options but not rev arguments (they are fixed to the
commits under bisection).
__git_complete_log_opts has a precondition that COMPREPLY be empty. In
a completion context it doesn't seem advisable to implement
preconditions as noisy or hard failures, so instead it becomes a no-op
on violation. This should be detectable and quick to debug for devels,
without ever aggravating a user (besides completion failure).
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 30 +++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 15d22ff7d9..c16aded36c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1472,6 +1472,16 @@ _git_bisect ()
;;
esac
;;
+ visualize)
+ case "$cur" in
+ -*)
+ __git_complete_log_opts
+ return
+ ;;
+ *)
+ ;;
+ esac
+ ;;
esac
case "$subcommand" in
@@ -2074,10 +2084,14 @@ __git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-c
__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-_git_log ()
+
+# Check for only porcelain (i.e. not git-rev-list) option (not argument)
+# and selected option argument completions for git-log options and if any
+# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
+# and will be empty on return if no candidates are found.
+__git_complete_log_opts ()
{
- __git_has_doubledash && return
- __git_find_repo_path
+ [ -z "$COMPREPLY" ] || return 1 # Precondition
local merge=""
if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
@@ -2171,6 +2185,16 @@ _git_log ()
return
;;
esac
+}
+
+_git_log ()
+{
+ __git_has_doubledash && return
+ __git_find_repo_path
+
+ __git_complete_log_opts
+ [ -z "$COMPREPLY" ] || return
+
__git_complete_revlist
}
--
2.43.0
^ permalink raw reply related
* [PATCH v3 0/5] completion: improvements for git-bisect
From: Britton Leo Kerin @ 2024-01-18 20:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <03fe3371-2b0f-4590-90ad-166b8fa4cbbb@smtp-relay.sendinblue.com>
CC to add: CC: Junio C Hamano <gitster@pobox.com>, Patrick Steinhardt <ps@pks.im>
Relative to v2 this only changes a wrong misleading commit message.
Bring completion for bisect up to date with current features.
Britton Leo Kerin (5):
completion: complete new old actions, start opts
completion: git-log opts to bisect visualize
completion: move to maintain define-before-use
completion: custom git-bisect terms
completion: git-bisect view recognized but not completed
contrib/completion/git-completion.bash | 312 +++++++++++++++----------
1 file changed, 183 insertions(+), 129 deletions(-)
Range-diff against v2:
1: e16264bfb9 = 1: e16264bfb9 completion: complete new old actions, start opts
2: 130abe3460 = 2: 130abe3460 completion: git-log opts to bisect visualize
3: d659ace9c2 = 3: d659ace9c2 completion: move to maintain define-before-use
4: c5bee633b2 = 4: c5bee633b2 completion: custom git-bisect terms
5: 220650f0ba ! 5: 2bd0cb26f1 completion: custom git-bisect terms
@@ Metadata
Author: Britton Leo Kerin <britton.kerin@gmail.com>
## Commit message ##
- completion: custom git-bisect terms
+ completion: git-bisect view recognized but not completed
+
+ This allows the git-log options to be completed but avoids completion
+ ambiguity between visualize and view.
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
--
2.43.0
^ permalink raw reply
* [PATCH v3 1/5] completion: complete new old actions, start opts
From: Britton Leo Kerin @ 2024-01-18 20:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240118204323.1113859-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 185b47d802..15d22ff7d9 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1449,7 +1449,7 @@ _git_bisect ()
{
__git_has_doubledash && return
- local subcommands="start bad good skip reset visualize replay log run"
+ local subcommands="start bad new good old terms skip reset visualize replay log run help"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__git_find_repo_path
@@ -1462,7 +1462,20 @@ _git_bisect ()
fi
case "$subcommand" in
- bad|good|reset|skip|start)
+ start)
+ case "$cur" in
+ --*)
+ __gitcomp "--term-new --term-bad --term-old --term-good --first-parent --no-checkout"
+ return
+ ;;
+ *)
+ ;;
+ esac
+ ;;
+ esac
+
+ case "$subcommand" in
+ bad|new|good|old|reset|skip|start)
__git_complete_refs
;;
*)
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Junio C Hamano @ 2024-01-18 20:19 UTC (permalink / raw)
To: Dragan Simic; +Cc: Jeff King, Rubén Justo, Git List
In-Reply-To: <bfded0571f133ffc1612fcbca1811a43@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> On 2024-01-18 19:26, Junio C Hamano wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>>
>>> Perhaps having both options available could be a good thing. Though,
>>> adding quite a few knobs may end up confusing the users, so we should
>>> make sure to document it well.
>> I agree there is a need for documentation, but we are not increasing
>> the number of knobs, are we?
>
> Perhaps there would be one more knob if we end up deciding to implement
> support for both approaches, as previously discussed.
But that would be just one knob (which I personally do not quite see
the need for), not "quite a few knobs" as you said, no?
^ 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