* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
From: Jeff King @ 2023-09-16 5:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: Rubén Justo, Git List,
Ævar Arnfjörð Bjarmason
In-Reply-To: <xmqq5y4bfhpm.fsf@gitster.g>
On Fri, Sep 15, 2023 at 10:51:17AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > But having done so, the main value in re-rolling would be preventing
> > somebody else from reading the code and having the same question.
>
> Indeed. It would be valuable to help future developers not to waste
> time on wondering what we already know they may do so on.
I guess I was thinking "maybe that is not a good reason to change what
the code does" when I wrote my other email. But it is probably a good
reason to make a note in the commit message. :)
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0
From: Jeff King @ 2023-09-16 5:32 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZQSkjiyrOac4DK8q@nand.local>
On Fri, Sep 15, 2023 at 02:38:06PM -0400, Taylor Blau wrote:
> This looks good, too, though I do have one question. The HTTP/2
> specification in 5.1 says (among other things):
>
> Streams are identified with an unsigned 31-bit integer. Streams
> initiated by a client MUST use odd-numbered stream identifiers; those
> initiated by the server MUST use even-numbered stream identifiers. A
> stream identifier of zero (0x0) is used for connection control messages;
> the stream identifier of zero cannot be used to establish a new stream.
>
> So the parsing you wrote here makes sense in that we consume digits
> between the pair of square brackets enclosing the stream identifier.
Yes, though I'm less concerned with what the standard says than with
what curl's code does (and it uses %d).
> But I think we would happily eat a line like:
>
> [HTTP/2] [] [Secret: xyz]
>
> even lacking a stream identifier. I think that's reasonably OK in
> practice, because we're being over-eager in redacting instead of the
> other way around. And we're unlikely to see such a line from curl
> anyway, so I don't think that it matters.
Yes, you're correct that we'd allow an empty stream identifier. I'm
content to leave it in the name of simplicity.
> If you feel otherwise, though, I think something as simple as:
>
> if (skip_iprefix(line, "[HTTP/2] [", &p)) {
> if (!*p)
> return 0;
> while (isdigit(*p))
> p++;
> if (skip_prefix(p, "] [", out))
> return 1;
> }
Yes, that would work, but...
> would do the trick. I *think* that this would also work:
>
> if (skip_iprefix(line, "[HTTP/2] [", &p)) {
> do {
> p++;
> } while (isdigit(*p))
> if (skip_prefix(p, "] [", out))
> return 1;
> }
>
> since we know that p is non-NULL, and if it's the end of the line, *p
> will be NUL and isdigit(*p) will return 0. But it's arguably less
> direct, and requires some extra reasoning, so I have a vague preference
> for the former.
Your do-while is too eager, I think. It advances the first "p" before
we've looked at it, so:
- we'd match "[HTTP/2] [x1] [foo]", allowing one byte of non-digit
cruft
- if the string is "[HTTP/2] [", then "p" is at the NUL after the
skip_iprefix call, and p++ walks us off the end of the array.
> But this may all be moot anyway, I don't feel strongly one way or the
> other.
My inclination is to leave it. I was actually tempted to just allow
_anything_ in the brackets if only because it makes the code even
simpler, but the "skip past digits" seemed like a reasonable middle
ground.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0
From: Jeff King @ 2023-09-16 5:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqsf7fe1q4.fsf@gitster.g>
On Fri, Sep 15, 2023 at 11:21:55AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > @@ -751,6 +753,18 @@ static int match_curl_h2_trace(const char *line, const char **out)
> > skip_iprefix(line, "h2 [", out))
> > return 1;
> >
> > + /*
> > + * curl 8.3.0 uses:
> > + * [HTTP/2] [<stream-id>] [<header-name>: <header-val>]
> > + * where <stream-id> is numeric.
> > + */
> > + if (skip_iprefix(line, "[HTTP/2] [", &p)) {
> > + while (isdigit(*p))
> > + p++;
> > + if (skip_prefix(p, "] [", out))
> > + return 1;
> > + }
>
> Looking good assuming that <stream-id> part will never be updated to
> allow spaces around the ID, or allow non-digits in the ID, in the
> future. Is there much harm if this code allowed false positives and
> sent something that is *not* a curl trace, like "foo]" parsed out of
> "[HTTP/2] [PATCH] [foo]", to redact_sensitive_header() function?
The current code on the generating side is pretty strict. It's
literally a printf using "[HTTP/2] [%d] [%.*s: %.*s]". As far as future
changes, I'm hesitant to make any changes based on guesses of what
_could_ happen. Our chance of hitting the mark is not high (I never
would have dreamed about this format after seeing the existing h2h3
ones), and it always carries the risk of misinterpretation.
You are right that the cost of a false positive is probably not too high
(the absolute worst case is that we redact something that looks
header-ish in the trace output). But even still, I'd prefer not to
complicate the code with extra parsing for a format that may or may not
ever come to exist.
If we were to loosen the parsing, it would make more sense to me to
loosen _much_ more, and just look for anything inside brackets.
Something like:
p = header->buf;
while ((p = strchr(p, '['))) {
if (redact_sensitive_header(header, p - header->buf + 1)) {
/* redaction ate our closing bracket */
strbuf_addch(header, ']');
break;
}
p++; /* skip past to look for next opening bracket */
}
Then we are relying on redact_sensitive_header() to match the header
strings, and we'll pass it lots of garbage which it will reject. But at
least we've bought something: all of the h2 formats we know about will
just work, and any future ones which retain the bracketing will as well.
That said, I'm still somewhat inclined to the stricter parsing, just
because it's possible for us to see arbitrary bytes here. So if you had
a header that happened to have brackets in it, we'd match those.
Probably nothing too bad could come of it, but it just feels sloppy to
me.
> By the way, would this patch make sense? Everybody in the function
> that try to notice a sensitive header seems to check the sentting
> independently, which seems error prone for those who want to add a
> new header to redact.
> [...]
> + if (!trace_curl_redact)
> + return ret;
Yeah, that looks a reasonable simplification to me (though obviously
orthogonal to the patch under discussion).
-Peff
^ permalink raw reply
* Re: BUG: git-gui no longer executes hook scripts
From: Junio C Hamano @ 2023-09-16 4:45 UTC (permalink / raw)
To: Mark Levedahl; +Cc: git, johannes.schindelin, me
In-Reply-To: <454d8b7b-96df-ec8f-2285-e022de66c66c@gmail.com>
Mark Levedahl <mlevedahl@gmail.com> writes:
> I think a simpler fix is just to examine the number of path components
> - more than one means a relative or absolute command (/foo splits into
> two parts). The below works for me on Linux.
That is clever, but I cannot convince myself that it is not too
clever for its own sake. The "pathtype" thing Dscho used in his
original is documented to be aware of things like "C:\path\name",
but I didn't re-read the Tcl manual page too carefully to know what
"file split" does for such pathname to be certain.
^ permalink raw reply
* Re: [PATCH v4] merge-tree: add -X strategy option
From: 唐宇奕 @ 2023-09-16 4:04 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <pull.1565.v4.git.1694836025469.gitgitgadget@gmail.com>
Thanks, what we want to do is to compare two patchsets' diff while
providing capabilities like normal commit diff.
The existing range-diff command provide an effective solution, however:
* The output isn't suitable for machine processing
* The command take's number of commits and commit messages into count,
while what we really want is merely the content diff
* The command doesn't support features like word diff, ignoring
whitespace changes, etc
So what comes to our mind is to simulate user behavior. We first merge
the old patchset into the new patchset's base commit, then use the
merge result to diff against the new patchset's source commit.
By doing this, the diff introduced between two patchsets' bases won't
be shown. Thus we get the 'real diff' between two patchsets.
In the first step, we use git-merge-tree to produce the merge result
since its performance's better than git-merge.
However, sometimes there's conflict between the new patchset's base
and the old patchset's source.So we need automatic conflict resolving
- only use content from 'their' side specifically.
Hope that makes sense.
On Sat, Sep 16, 2023 at 11:47 AM Izzy via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Tang Yuyi <winglovet@gmail.com>
>
> Add merge strategy option to produce more customizable merge result such
> as automatically resolving conflicts.
>
> Signed-off-by: Tang Yuyi <winglovet@gmail.com>
> ---
> merge-tree: add -X strategy option
>
> Change-Id: I16be592262d13cebcff8726eb043f7ecdb313b76
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1565%2FWingT%2Fmerge_tree_allow_strategy_option-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/1565
>
> Range-diff vs v3:
>
> 1: d64a774fa7c ! 1: d2d8fcc2e9b merge-tree: add -X strategy option
> @@ Commit message
> merge-tree: add -X strategy option
>
> Add merge strategy option to produce more customizable merge result such
> - as automatically solve conflicts.
> + as automatically resolving conflicts.
>
> Signed-off-by: Tang Yuyi <winglovet@gmail.com>
>
>
>
> builtin/merge-tree.c | 24 ++++++++++++++++++++++++
> t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++
> 2 files changed, 47 insertions(+)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 0de42aecf4b..2ec6ec0d39a 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -19,6 +19,8 @@
> #include "tree.h"
> #include "config.h"
>
> +static const char **xopts;
> +static size_t xopts_nr, xopts_alloc;
> static int line_termination = '\n';
>
> struct merge_list {
> @@ -414,6 +416,7 @@ struct merge_tree_options {
> int show_messages;
> int name_only;
> int use_stdin;
> + struct merge_options merge_options;
> };
>
> static int real_merge(struct merge_tree_options *o,
> @@ -439,6 +442,8 @@ static int real_merge(struct merge_tree_options *o,
>
> init_merge_options(&opt, the_repository);
>
> + opt.recursive_variant = o->merge_options.recursive_variant;
> +
> opt.show_rename_progress = 0;
>
> opt.branch1 = branch1;
> @@ -510,6 +515,17 @@ static int real_merge(struct merge_tree_options *o,
> return !result.clean; /* result.clean < 0 handled above */
> }
>
> +static int option_parse_x(const struct option *opt,
> + const char *arg, int unset)
> +{
> + if (unset)
> + return 0;
> +
> + ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
> + xopts[xopts_nr++] = xstrdup(arg);
> + return 0;
> +}
> +
> int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> {
> struct merge_tree_options o = { .show_messages = -1 };
> @@ -548,6 +564,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> &merge_base,
> N_("commit"),
> N_("specify a merge-base for the merge")),
> + OPT_CALLBACK('X', "strategy-option", &xopts,
> + N_("option=value"),
> + N_("option for selected merge strategy"),
> + option_parse_x),
> OPT_END()
> };
>
> @@ -556,6 +576,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, mt_options,
> merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>
> + for (int x = 0; x < xopts_nr; x++)
> + if (parse_merge_opt(&o.merge_options, xopts[x]))
> + die(_("unknown strategy option: -X%s"), xopts[x]);
> +
> /* Handle --stdin */
> if (o.use_stdin) {
> struct strbuf buf = STRBUF_INIT;
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index 250f721795b..4125bb101ec 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -22,6 +22,7 @@ test_expect_success setup '
> git branch side1 &&
> git branch side2 &&
> git branch side3 &&
> + git branch side4 &&
>
> git checkout side1 &&
> test_write_lines 1 2 3 4 5 6 >numbers &&
> @@ -46,6 +47,13 @@ test_expect_success setup '
> test_tick &&
> git commit -m rename-numbers &&
>
> + git checkout side4 &&
> + test_write_lines 0 1 2 3 4 5 >numbers &&
> + echo yo >greeting &&
> + git add numbers greeting &&
> + test_tick &&
> + git commit -m other-content-modifications &&
> +
> git switch --orphan unrelated &&
> >something-else &&
> git add something-else &&
> @@ -97,6 +105,21 @@ test_expect_success 'Content merge and a few conflicts' '
> test_cmp expect actual
> '
>
> +test_expect_success 'Auto resolve conflicts by "ours" strategy option' '
> + git checkout side1^0 &&
> +
> + # make sure merge conflict exists
> + test_must_fail git merge side4 &&
> + git merge --abort &&
> +
> + git merge -X ours side4 &&
> + git rev-parse HEAD^{tree} > expected &&
> +
> + git merge-tree -X ours side1 side4 > actual &&
> +
> + test_cmp expected actual
> +'
> +
> test_expect_success 'Barf on misspelled option, with exit code other than 0 or 1' '
> # Mis-spell with single "s" instead of double "s"
> test_expect_code 129 git merge-tree --write-tree --mesages FOOBAR side1 side2 2>expect &&
>
> base-commit: ac83bc5054c2ac489166072334b4147ce6d0fccb
> --
> gitgitgadget
^ permalink raw reply
* Re: [PATCH v4] merge-tree: add -X strategy option
From: Elijah Newren @ 2023-09-16 3:55 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, Tang Yuyi
In-Reply-To: <pull.1565.v4.git.1694836025469.gitgitgadget@gmail.com>
Hi,
On Fri, Sep 15, 2023 at 8:47 PM Izzy via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Tang Yuyi <winglovet@gmail.com>
>
> Add merge strategy option to produce more customizable merge result such
> as automatically resolving conflicts.
>
> Signed-off-by: Tang Yuyi <winglovet@gmail.com>
> ---
> merge-tree: add -X strategy option
>
> Change-Id: I16be592262d13cebcff8726eb043f7ecdb313b76
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1565%2FWingT%2Fmerge_tree_allow_strategy_option-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/1565
>
> Range-diff vs v3:
>
> 1: d64a774fa7c ! 1: d2d8fcc2e9b merge-tree: add -X strategy option
> @@ Commit message
> merge-tree: add -X strategy option
>
> Add merge strategy option to produce more customizable merge result such
> - as automatically solve conflicts.
> + as automatically resolving conflicts.
Thanks for fixing this part of what I pointed out in the review.
> Signed-off-by: Tang Yuyi <winglovet@gmail.com>
>
>
>
> builtin/merge-tree.c | 24 ++++++++++++++++++++++++
> t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++
> 2 files changed, 47 insertions(+)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 0de42aecf4b..2ec6ec0d39a 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -19,6 +19,8 @@
> #include "tree.h"
> #include "config.h"
>
> +static const char **xopts;
> +static size_t xopts_nr, xopts_alloc;
> static int line_termination = '\n';
>
> struct merge_list {
> @@ -414,6 +416,7 @@ struct merge_tree_options {
> int show_messages;
> int name_only;
> int use_stdin;
> + struct merge_options merge_options;
> };
>
> static int real_merge(struct merge_tree_options *o,
> @@ -439,6 +442,8 @@ static int real_merge(struct merge_tree_options *o,
>
> init_merge_options(&opt, the_repository);
>
> + opt.recursive_variant = o->merge_options.recursive_variant;
> +
> opt.show_rename_progress = 0;
>
> opt.branch1 = branch1;
> @@ -510,6 +515,17 @@ static int real_merge(struct merge_tree_options *o,
> return !result.clean; /* result.clean < 0 handled above */
> }
>
> +static int option_parse_x(const struct option *opt,
> + const char *arg, int unset)
> +{
> + if (unset)
> + return 0;
> +
> + ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
> + xopts[xopts_nr++] = xstrdup(arg);
> + return 0;
> +}
> +
> int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> {
> struct merge_tree_options o = { .show_messages = -1 };
> @@ -548,6 +564,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> &merge_base,
> N_("commit"),
> N_("specify a merge-base for the merge")),
> + OPT_CALLBACK('X', "strategy-option", &xopts,
> + N_("option=value"),
> + N_("option for selected merge strategy"),
> + option_parse_x),
> OPT_END()
> };
>
> @@ -556,6 +576,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, mt_options,
> merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>
> + for (int x = 0; x < xopts_nr; x++)
> + if (parse_merge_opt(&o.merge_options, xopts[x]))
> + die(_("unknown strategy option: -X%s"), xopts[x]);
> +
> /* Handle --stdin */
> if (o.use_stdin) {
> struct strbuf buf = STRBUF_INIT;
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index 250f721795b..4125bb101ec 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -22,6 +22,7 @@ test_expect_success setup '
> git branch side1 &&
> git branch side2 &&
> git branch side3 &&
> + git branch side4 &&
>
> git checkout side1 &&
> test_write_lines 1 2 3 4 5 6 >numbers &&
> @@ -46,6 +47,13 @@ test_expect_success setup '
> test_tick &&
> git commit -m rename-numbers &&
>
> + git checkout side4 &&
> + test_write_lines 0 1 2 3 4 5 >numbers &&
> + echo yo >greeting &&
> + git add numbers greeting &&
> + test_tick &&
> + git commit -m other-content-modifications &&
> +
> git switch --orphan unrelated &&
> >something-else &&
> git add something-else &&
> @@ -97,6 +105,21 @@ test_expect_success 'Content merge and a few conflicts' '
> test_cmp expect actual
> '
>
> +test_expect_success 'Auto resolve conflicts by "ours" strategy option' '
> + git checkout side1^0 &&
> +
> + # make sure merge conflict exists
> + test_must_fail git merge side4 &&
> + git merge --abort &&
> +
> + git merge -X ours side4 &&
> + git rev-parse HEAD^{tree} > expected &&
> +
> + git merge-tree -X ours side1 side4 > actual &&
Please fix the problems in these last two lines as pointed out in both
of my last two reviews.
^ permalink raw reply
* [PATCH v4] merge-tree: add -X strategy option
From: Izzy via GitGitGadget @ 2023-09-16 3:47 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Izzy, Tang Yuyi
In-Reply-To: <pull.1565.v3.git.1694830462463.gitgitgadget@gmail.com>
From: Tang Yuyi <winglovet@gmail.com>
Add merge strategy option to produce more customizable merge result such
as automatically resolving conflicts.
Signed-off-by: Tang Yuyi <winglovet@gmail.com>
---
merge-tree: add -X strategy option
Change-Id: I16be592262d13cebcff8726eb043f7ecdb313b76
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1565%2FWingT%2Fmerge_tree_allow_strategy_option-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1565
Range-diff vs v3:
1: d64a774fa7c ! 1: d2d8fcc2e9b merge-tree: add -X strategy option
@@ Commit message
merge-tree: add -X strategy option
Add merge strategy option to produce more customizable merge result such
- as automatically solve conflicts.
+ as automatically resolving conflicts.
Signed-off-by: Tang Yuyi <winglovet@gmail.com>
builtin/merge-tree.c | 24 ++++++++++++++++++++++++
t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 0de42aecf4b..2ec6ec0d39a 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -19,6 +19,8 @@
#include "tree.h"
#include "config.h"
+static const char **xopts;
+static size_t xopts_nr, xopts_alloc;
static int line_termination = '\n';
struct merge_list {
@@ -414,6 +416,7 @@ struct merge_tree_options {
int show_messages;
int name_only;
int use_stdin;
+ struct merge_options merge_options;
};
static int real_merge(struct merge_tree_options *o,
@@ -439,6 +442,8 @@ static int real_merge(struct merge_tree_options *o,
init_merge_options(&opt, the_repository);
+ opt.recursive_variant = o->merge_options.recursive_variant;
+
opt.show_rename_progress = 0;
opt.branch1 = branch1;
@@ -510,6 +515,17 @@ static int real_merge(struct merge_tree_options *o,
return !result.clean; /* result.clean < 0 handled above */
}
+static int option_parse_x(const struct option *opt,
+ const char *arg, int unset)
+{
+ if (unset)
+ return 0;
+
+ ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
+ xopts[xopts_nr++] = xstrdup(arg);
+ return 0;
+}
+
int cmd_merge_tree(int argc, const char **argv, const char *prefix)
{
struct merge_tree_options o = { .show_messages = -1 };
@@ -548,6 +564,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
&merge_base,
N_("commit"),
N_("specify a merge-base for the merge")),
+ OPT_CALLBACK('X', "strategy-option", &xopts,
+ N_("option=value"),
+ N_("option for selected merge strategy"),
+ option_parse_x),
OPT_END()
};
@@ -556,6 +576,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, mt_options,
merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+ for (int x = 0; x < xopts_nr; x++)
+ if (parse_merge_opt(&o.merge_options, xopts[x]))
+ die(_("unknown strategy option: -X%s"), xopts[x]);
+
/* Handle --stdin */
if (o.use_stdin) {
struct strbuf buf = STRBUF_INIT;
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 250f721795b..4125bb101ec 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -22,6 +22,7 @@ test_expect_success setup '
git branch side1 &&
git branch side2 &&
git branch side3 &&
+ git branch side4 &&
git checkout side1 &&
test_write_lines 1 2 3 4 5 6 >numbers &&
@@ -46,6 +47,13 @@ test_expect_success setup '
test_tick &&
git commit -m rename-numbers &&
+ git checkout side4 &&
+ test_write_lines 0 1 2 3 4 5 >numbers &&
+ echo yo >greeting &&
+ git add numbers greeting &&
+ test_tick &&
+ git commit -m other-content-modifications &&
+
git switch --orphan unrelated &&
>something-else &&
git add something-else &&
@@ -97,6 +105,21 @@ test_expect_success 'Content merge and a few conflicts' '
test_cmp expect actual
'
+test_expect_success 'Auto resolve conflicts by "ours" strategy option' '
+ git checkout side1^0 &&
+
+ # make sure merge conflict exists
+ test_must_fail git merge side4 &&
+ git merge --abort &&
+
+ git merge -X ours side4 &&
+ git rev-parse HEAD^{tree} > expected &&
+
+ git merge-tree -X ours side1 side4 > actual &&
+
+ test_cmp expected actual
+'
+
test_expect_success 'Barf on misspelled option, with exit code other than 0 or 1' '
# Mis-spell with single "s" instead of double "s"
test_expect_code 129 git merge-tree --write-tree --mesages FOOBAR side1 side2 2>expect &&
base-commit: ac83bc5054c2ac489166072334b4147ce6d0fccb
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v3] merge-tree: add -X strategy option
From: Elijah Newren @ 2023-09-16 3:21 UTC (permalink / raw)
To: 唐宇奕; +Cc: Izzy via GitGitGadget, git
In-Reply-To: <CAFWsj_UeK-5uyCtBfLYeRZXRMdAD_eFassxEo6FvgeVzmwqLNQ@mail.gmail.com>
On Fri, Sep 15, 2023 at 7:27 PM 唐宇奕 <winglovet@gmail.com> wrote:
>
> Thanks for your advice!
> I've fixed those blocking issues.
Thanks, you did fix most of them. I left a comment on a few things on V3.
> However, regarding the global variable issue, I'm not familiar with C
> and git code and don't know how to solve this. I think perhaps we need
> something like closure to parse opt into a local variable?
You could merely make xopts, xopts_nr, and xopts_alloc local
variables, and pass them around as needed. Closures would make things
look cleaner perhaps, but certainly aren't necessarily.
But, I don't think the globals thing is a blocking issue, especially
since we already have a number of unnecessary globals in that file
already.
> Our usecase is to achieve something like 'range-diff', we first use
> merge-tree to merge new patchset's base commit with old patchset's
> source commit, then use the merge result to diff against new
> patchset's source commit. So we only need to make sure conflict's are
> handled automatically, leaving other diff features to the second step.
If you're trying to do something like range-diff, then I don't see why
there's any point in creating a merge at all. I'm afraid I don't
follow your explanations about the steps you are taking and more
importantly why you are taking them, and how it helps you achieve your
goal of doing "something like 'range-diff'". Could you elaborate?
^ permalink raw reply
* Re: [PATCH v3] merge-tree: add -X strategy option
From: Elijah Newren @ 2023-09-16 3:16 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: Git Mailing List, Tang Yuyi
In-Reply-To: <pull.1565.v3.git.1694830462463.gitgitgadget@gmail.com>
Hi,
On Fri, Sep 15, 2023 at 7:14 PM Izzy via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Tang Yuyi <winglovet@gmail.com>
>
> Add merge strategy option to produce more customizable merge result such
> as automatically solve conflicts.
s/solve/resolving/
I think "solve" should be "solving" here, except that "solve" isn't
really the right word. It's not solving (figuring out) anything, it's
using a big hammer to make the problem just go away. So, I think
"resolving" is a better choice.
> Signed-off-by: Tang Yuyi <winglovet@gmail.com>
> ---
> merge-tree: add -X strategy option
>
> Change-Id: I16be592262d13cebcff8726eb043f7ecdb313b76
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1565%2FWingT%2Fmerge_tree_allow_strategy_option-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/1565
>
> Range-diff vs v2:
>
> 1: 7d53d08381e ! 1: d64a774fa7c merge-tree: add -X strategy option
> @@
> ## Metadata ##
> -Author: winglovet <winglovet@gmail.com>
> +Author: Tang Yuyi <winglovet@gmail.com>
>
> ## Commit message ##
> merge-tree: add -X strategy option
> @@ Commit message
> Add merge strategy option to produce more customizable merge result such
> as automatically solve conflicts.
>
> - Signed-off-by: winglovet <winglovet@gmail.com>
> + Signed-off-by: Tang Yuyi <winglovet@gmail.com>
>
> ## builtin/merge-tree.c ##
> @@
> @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Content merge and a few c
> test_cmp expect actual
> '
>
> -+test_expect_success 'Auto resolve conflicts by "ours" stragety option' '
> ++test_expect_success 'Auto resolve conflicts by "ours" strategy option' '
> + git checkout side1^0 &&
> +
> + # make sure merge conflict exists
>
>
> builtin/merge-tree.c | 24 ++++++++++++++++++++++++
> t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++
> 2 files changed, 47 insertions(+)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 0de42aecf4b..2ec6ec0d39a 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -19,6 +19,8 @@
> #include "tree.h"
> #include "config.h"
>
> +static const char **xopts;
> +static size_t xopts_nr, xopts_alloc;
> static int line_termination = '\n';
>
> struct merge_list {
> @@ -414,6 +416,7 @@ struct merge_tree_options {
> int show_messages;
> int name_only;
> int use_stdin;
> + struct merge_options merge_options;
> };
>
> static int real_merge(struct merge_tree_options *o,
> @@ -439,6 +442,8 @@ static int real_merge(struct merge_tree_options *o,
>
> init_merge_options(&opt, the_repository);
>
> + opt.recursive_variant = o->merge_options.recursive_variant;
> +
> opt.show_rename_progress = 0;
>
> opt.branch1 = branch1;
> @@ -510,6 +515,17 @@ static int real_merge(struct merge_tree_options *o,
> return !result.clean; /* result.clean < 0 handled above */
> }
>
> +static int option_parse_x(const struct option *opt,
> + const char *arg, int unset)
> +{
> + if (unset)
> + return 0;
> +
> + ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
> + xopts[xopts_nr++] = xstrdup(arg);
> + return 0;
> +}
> +
> int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> {
> struct merge_tree_options o = { .show_messages = -1 };
> @@ -548,6 +564,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> &merge_base,
> N_("commit"),
> N_("specify a merge-base for the merge")),
> + OPT_CALLBACK('X', "strategy-option", &xopts,
> + N_("option=value"),
> + N_("option for selected merge strategy"),
> + option_parse_x),
> OPT_END()
> };
>
> @@ -556,6 +576,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, mt_options,
> merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>
> + for (int x = 0; x < xopts_nr; x++)
> + if (parse_merge_opt(&o.merge_options, xopts[x]))
> + die(_("unknown strategy option: -X%s"), xopts[x]);
> +
> /* Handle --stdin */
> if (o.use_stdin) {
> struct strbuf buf = STRBUF_INIT;
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index 250f721795b..4125bb101ec 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -22,6 +22,7 @@ test_expect_success setup '
> git branch side1 &&
> git branch side2 &&
> git branch side3 &&
> + git branch side4 &&
>
> git checkout side1 &&
> test_write_lines 1 2 3 4 5 6 >numbers &&
> @@ -46,6 +47,13 @@ test_expect_success setup '
> test_tick &&
> git commit -m rename-numbers &&
>
> + git checkout side4 &&
> + test_write_lines 0 1 2 3 4 5 >numbers &&
> + echo yo >greeting &&
> + git add numbers greeting &&
> + test_tick &&
> + git commit -m other-content-modifications &&
> +
> git switch --orphan unrelated &&
> >something-else &&
> git add something-else &&
> @@ -97,6 +105,21 @@ test_expect_success 'Content merge and a few conflicts' '
> test_cmp expect actual
> '
>
> +test_expect_success 'Auto resolve conflicts by "ours" strategy option' '
> + git checkout side1^0 &&
> +
> + # make sure merge conflict exists
> + test_must_fail git merge side4 &&
> + git merge --abort &&
> +
> + git merge -X ours side4 &&
> + git rev-parse HEAD^{tree} > expected &&
> +
> + git merge-tree -X ours side1 side4 > actual &&
Multiple style problems still here from V2:
* There should be no space between the redirection operator ('>')
and the filename following it.
* You have indented most lines with tabs, but the line just above
this one with 4 spaces.
> +
> + test_cmp expected actual
> +'
> +
> test_expect_success 'Barf on misspelled option, with exit code other than 0 or 1' '
> # Mis-spell with single "s" instead of double "s"
> test_expect_code 129 git merge-tree --write-tree --mesages FOOBAR side1 side2 2>expect &&
>
> base-commit: ac83bc5054c2ac489166072334b4147ce6d0fccb
^ permalink raw reply
* Re: [PATCH 4/4] merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()
From: Elijah Newren @ 2023-09-16 3:09 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20230914094004.GD2254894@coredump.intra.peff.net>
On Thu, Sep 14, 2023 at 2:40 AM Jeff King <peff@peff.net> wrote:
>
> The merge_options parameter has never been used since the function was
> introduced in 64aceb6d73 (merge-ort: add code to check for whether
> cached renames can be reused, 2021-05-20). In theory some merge options
> might impact our decisions here, but that has never been the case so
> far.
Yeah, it was used in some preliminary versions of the code while I was
developing the new algorithm, but there were lots of changes between
when I started working on merge-ort and when it was finally ready to
submit for review. I must have just overlooked that this parameter
was no longer needed. Thanks for catching and cleaning up.
> Let's drop it to appease -Wunused-parameter; it would be easy to add
> back later if we need to (there is only one caller).
Yep, makes sense.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> merge-ort.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 20eefd9b5e..3953c9f745 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4880,8 +4880,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
> trace2_region_leave("merge", "allocate/init", opt->repo);
> }
>
> -static void merge_check_renames_reusable(struct merge_options *opt,
> - struct merge_result *result,
> +static void merge_check_renames_reusable(struct merge_result *result,
> struct tree *merge_base,
> struct tree *side1,
> struct tree *side2)
> @@ -5083,7 +5082,7 @@ void merge_incore_nonrecursive(struct merge_options *opt,
>
> trace2_region_enter("merge", "merge_start", opt->repo);
> assert(opt->ancestor != NULL);
> - merge_check_renames_reusable(opt, result, merge_base, side1, side2);
> + merge_check_renames_reusable(result, merge_base, side1, side2);
> merge_start(opt, result);
> /*
> * Record the trees used in this merge, so if there's a next merge in
> --
> 2.42.0.628.g8a27295885
^ permalink raw reply
* Re: [PATCH 3/4] merge-ort: drop unused parameters from detect_and_process_renames()
From: Elijah Newren @ 2023-09-16 3:04 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20230914093958.GC2254894@coredump.intra.peff.net>
On Thu, Sep 14, 2023 at 2:39 AM Jeff King <peff@peff.net> wrote:
>
> This function takes three trees representing the merge base and both
> sides of the merge, but never looks at any of them. This is due to
> f78cf97617 (merge-ort: call diffcore_rename() directly, 2021-02-14).
> Prior to that commit, we passed pairs of trees to diff_tree_oid(). But
> after that commit, we collect a custom diff_queue for each pair in the
> merge_options struct, and just run diffcore_rename() on the result. So
> the function does not need to know about the original trees at all
> anymore.
Thanks for including the history.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> merge-ort.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 31c663b297..20eefd9b5e 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3324,10 +3324,7 @@ static int collect_renames(struct merge_options *opt,
> return clean;
> }
>
> -static int detect_and_process_renames(struct merge_options *opt,
> - struct tree *merge_base,
> - struct tree *side1,
> - struct tree *side2)
> +static int detect_and_process_renames(struct merge_options *opt)
> {
> struct diff_queue_struct combined = { 0 };
> struct rename_info *renames = &opt->priv->renames;
> @@ -4964,8 +4961,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
> trace2_region_leave("merge", "collect_merge_info", opt->repo);
>
> trace2_region_enter("merge", "renames", opt->repo);
> - result->clean = detect_and_process_renames(opt, merge_base,
> - side1, side2);
> + result->clean = detect_and_process_renames(opt);
> trace2_region_leave("merge", "renames", opt->repo);
> if (opt->priv->renames.redo_after_renames == 2) {
> trace2_region_enter("merge", "reset_maps", opt->repo);
> --
> 2.42.0.628.g8a27295885
Looks good.
^ permalink raw reply
* Re: [PATCH 0/4] merge-ort unused parameter cleanups
From: Elijah Newren @ 2023-09-16 2:56 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20230914093409.GA2254811@coredump.intra.peff.net>
On Thu, Sep 14, 2023 at 2:34 AM Jeff King <peff@peff.net> wrote:
>
> A few small cleanups for merge-ort collected from playing with
> -Wunused-parameter. The first one actually fixes a user-visible bug, and
> the rest are just cleanups.
>
> [1/4]: merge-ort: drop custom err() function
> [2/4]: merge-ort: stop passing "opt" to read_oid_strbuf()
> [3/4]: merge-ort: drop unused parameters from detect_and_process_renames()
> [4/4]: merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()
>
> merge-ort.c | 48 +++++++++++--------------------------------
> t/t6406-merge-attr.sh | 3 ++-
> 2 files changed, 14 insertions(+), 37 deletions(-)
>
> -Peff
All look good; thanks.
^ permalink raw reply
* Re: [PATCH 1/4] merge-ort: drop custom err() function
From: Elijah Newren @ 2023-09-16 2:54 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20230914093948.GA2254894@coredump.intra.peff.net>
On Thu, Sep 14, 2023 at 2:39 AM Jeff King <peff@peff.net> wrote:
>
> The merge-ort code has an err() function, but it's really just error()
> in disguise. It differs in two ways:
>
> 1. It takes a "struct merge_options" argument. But the function
> completely ignores it! We can simply remove it.
Oops, when I simplified the err() function copied from
merge-recursive.c in one way, I failed to notice that it enabled
further simplifications.
> 2. It formats the error string into a strbuf, prepending "error: ",
> and then feeds the result into error(). But this is wrong! The
> error() function already adds the prefix, so we end up with:
>
> error: error: Failed to execute internal merge
...and the same problem can be found in merge-recursive.c's err() function.
Not sure what current opinions on whether we should bother fixing
those up. I do intend on nuking merge-recursive.c, but I obviously
haven't had much Git time this year.
> So let's just drop this function entirely and call error() directly, as
> the functions are otherwise identical (note that they both always return
> -1).
>
> Presumably nobody noticed the bogus messages because they are quite hard
> to trigger (they are mostly internal errors reading and writing
> objects). However, one easy trigger is a custom merge driver which dies
> by signal; we have a test already here, but we were not checking the
> contents of stderr.
Thanks for catching this.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> A few of these messages starts with capital letters, which is unlike our
> usual error message style. I didn't clean that up here. We could do so
> on top,
There are two of these. In my defense, they were copied verbatim from
merge-recursive.c. And I, um, never noticed the problem over there
before copying. Or after.
> but I actually wonder if some of these ought to be using
> path_msg() and continuing instead, to give output closer to other
> conflict or error cases (e.g., conflicts caused by missing submodule
> objects). But I dunno. I guess these are all more clearly "woah,
> something is totally wrong" that we do not expect to happen, so it
> probably isn't a big deal to just abort.
Yeah, all callers of err()/error() are for things that should never
happen regardless of repository contents and should result in an
instant abort, whereas anything calling path_msg() is a conflict or
informational message that is expected for various kinds of repository
data -- these messages are accumulated and later shown.
Another distinction is that any call to path_msg() is associated to a
very specific path (or a few specific paths in special cases like
renames or add/add with conflict modes), whereas none of the calls to
err()/error() have a specific path they are about. This serves a few
purposes:
* We've had reports before that users get confused when there are
multiple conflict messages about a path and they do not occur
together. The structure of the merge machinery is such that it often
has to process conflicts by type and then by path, rather than by path
and then by type. If a merge has many conflicts, processing by type
and then by path, combined with printing as you go, naturally results
in cases where there are multiple conflict type messages for a single
path, but the messages are separated by dozens or hundreds of lines of
conflict messages about other paths. By accumulating and printing
later, at print time we can sort based on path and provide nicer
output (though renames and such might still cause some separation of
related messages).
* Accumulating and printing conflict & informational messages later
is also more friendly for use by other tools such as merge-tree or
rebase that may want to only conditionally print the messages or even
operate on the structured data (the specific paths and conflict types
recorded with them) in some special way. Dscho and I talked about
that for his webby-merge-ui-for-github tool he was working on.
Anyway, long story short is that I think continuing to use error()
instead of path_msg() or something else makes sense here. The capital
to lowercase cleanups make sense; we could even #leftoverbits for that
piece.
> merge-ort.c | 28 +++++-----------------------
> t/t6406-merge-attr.sh | 3 ++-
> 2 files changed, 7 insertions(+), 24 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 8631c99700..027ecc7f78 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -721,23 +721,6 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
> renames->callback_data_nr = renames->callback_data_alloc = 0;
> }
>
> -__attribute__((format (printf, 2, 3)))
> -static int err(struct merge_options *opt, const char *err, ...)
> -{
> - va_list params;
> - struct strbuf sb = STRBUF_INIT;
> -
> - strbuf_addstr(&sb, "error: ");
> - va_start(params, err);
> - strbuf_vaddf(&sb, err, params);
> - va_end(params);
> -
> - error("%s", sb.buf);
> - strbuf_release(&sb);
> -
> - return -1;
> -}
> -
> static void format_commit(struct strbuf *sb,
> int indent,
> struct repository *repo,
> @@ -2122,13 +2105,12 @@ static int handle_content_merge(struct merge_options *opt,
> &result_buf);
>
> if ((merge_status < 0) || !result_buf.ptr)
> - ret = err(opt, _("Failed to execute internal merge"));
> + ret = error(_("Failed to execute internal merge"));
>
> if (!ret &&
> write_object_file(result_buf.ptr, result_buf.size,
> OBJ_BLOB, &result->oid))
> - ret = err(opt, _("Unable to add %s to database"),
> - path);
> + ret = error(_("Unable to add %s to database"), path);
>
> free(result_buf.ptr);
> if (ret)
> @@ -3518,10 +3500,10 @@ static int read_oid_strbuf(struct merge_options *opt,
> unsigned long size;
> buf = repo_read_object_file(the_repository, oid, &type, &size);
> if (!buf)
> - return err(opt, _("cannot read object %s"), oid_to_hex(oid));
> + return error(_("cannot read object %s"), oid_to_hex(oid));
> if (type != OBJ_BLOB) {
> free(buf);
> - return err(opt, _("object %s is not a blob"), oid_to_hex(oid));
> + return error(_("object %s is not a blob"), oid_to_hex(oid));
> }
> strbuf_attach(dst, buf, size, size + 1);
> return 0;
> @@ -4973,7 +4955,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
> * TRANSLATORS: The %s arguments are: 1) tree hash of a merge
> * base, and 2-3) the trees for the two trees we're merging.
> */
> - err(opt, _("collecting merge info failed for trees %s, %s, %s"),
> + error(_("collecting merge info failed for trees %s, %s, %s"),
> oid_to_hex(&merge_base->object.oid),
> oid_to_hex(&side1->object.oid),
> oid_to_hex(&side2->object.oid));
> diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
> index 9677180a5b..05ad13b23e 100755
> --- a/t/t6406-merge-attr.sh
> +++ b/t/t6406-merge-attr.sh
> @@ -179,7 +179,8 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
>
> >./please-abort &&
> echo "* merge=custom" >.gitattributes &&
> - test_must_fail git merge main &&
> + test_must_fail git merge main 2>err &&
> + grep "^error: Failed to execute internal merge" err &&
> git ls-files -u >output &&
> git diff --name-only HEAD >>output &&
> test_must_be_empty output
> --
> 2.42.0.628.g8a27295885
Thanks for fixing this up.
^ permalink raw reply
* Re: [PATCH v3] merge-tree: add -X strategy option
From: 唐宇奕 @ 2023-09-16 2:26 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <pull.1565.v3.git.1694830462463.gitgitgadget@gmail.com>
Thanks for your advice!
I've fixed those blocking issues.
However, regarding the global variable issue, I'm not familiar with C
and git code and don't know how to solve this. I think perhaps we need
something like closure to parse opt into a local variable?
Our usecase is to achieve something like 'range-diff', we first use
merge-tree to merge new patchset's base commit with old patchset's
source commit, then use the merge result to diff against new
patchset's source commit. So we only need to make sure conflict's are
handled automatically, leaving other diff features to the second step.
On Sat, Sep 16, 2023 at 10:14 AM Izzy via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Tang Yuyi <winglovet@gmail.com>
>
> Add merge strategy option to produce more customizable merge result such
> as automatically solve conflicts.
>
> Signed-off-by: Tang Yuyi <winglovet@gmail.com>
> ---
> merge-tree: add -X strategy option
>
> Change-Id: I16be592262d13cebcff8726eb043f7ecdb313b76
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1565%2FWingT%2Fmerge_tree_allow_strategy_option-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/1565
>
> Range-diff vs v2:
>
> 1: 7d53d08381e ! 1: d64a774fa7c merge-tree: add -X strategy option
> @@
> ## Metadata ##
> -Author: winglovet <winglovet@gmail.com>
> +Author: Tang Yuyi <winglovet@gmail.com>
>
> ## Commit message ##
> merge-tree: add -X strategy option
> @@ Commit message
> Add merge strategy option to produce more customizable merge result such
> as automatically solve conflicts.
>
> - Signed-off-by: winglovet <winglovet@gmail.com>
> + Signed-off-by: Tang Yuyi <winglovet@gmail.com>
>
> ## builtin/merge-tree.c ##
> @@
> @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Content merge and a few c
> test_cmp expect actual
> '
>
> -+test_expect_success 'Auto resolve conflicts by "ours" stragety option' '
> ++test_expect_success 'Auto resolve conflicts by "ours" strategy option' '
> + git checkout side1^0 &&
> +
> + # make sure merge conflict exists
>
>
> builtin/merge-tree.c | 24 ++++++++++++++++++++++++
> t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++
> 2 files changed, 47 insertions(+)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 0de42aecf4b..2ec6ec0d39a 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -19,6 +19,8 @@
> #include "tree.h"
> #include "config.h"
>
> +static const char **xopts;
> +static size_t xopts_nr, xopts_alloc;
> static int line_termination = '\n';
>
> struct merge_list {
> @@ -414,6 +416,7 @@ struct merge_tree_options {
> int show_messages;
> int name_only;
> int use_stdin;
> + struct merge_options merge_options;
> };
>
> static int real_merge(struct merge_tree_options *o,
> @@ -439,6 +442,8 @@ static int real_merge(struct merge_tree_options *o,
>
> init_merge_options(&opt, the_repository);
>
> + opt.recursive_variant = o->merge_options.recursive_variant;
> +
> opt.show_rename_progress = 0;
>
> opt.branch1 = branch1;
> @@ -510,6 +515,17 @@ static int real_merge(struct merge_tree_options *o,
> return !result.clean; /* result.clean < 0 handled above */
> }
>
> +static int option_parse_x(const struct option *opt,
> + const char *arg, int unset)
> +{
> + if (unset)
> + return 0;
> +
> + ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
> + xopts[xopts_nr++] = xstrdup(arg);
> + return 0;
> +}
> +
> int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> {
> struct merge_tree_options o = { .show_messages = -1 };
> @@ -548,6 +564,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> &merge_base,
> N_("commit"),
> N_("specify a merge-base for the merge")),
> + OPT_CALLBACK('X', "strategy-option", &xopts,
> + N_("option=value"),
> + N_("option for selected merge strategy"),
> + option_parse_x),
> OPT_END()
> };
>
> @@ -556,6 +576,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, mt_options,
> merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>
> + for (int x = 0; x < xopts_nr; x++)
> + if (parse_merge_opt(&o.merge_options, xopts[x]))
> + die(_("unknown strategy option: -X%s"), xopts[x]);
> +
> /* Handle --stdin */
> if (o.use_stdin) {
> struct strbuf buf = STRBUF_INIT;
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index 250f721795b..4125bb101ec 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -22,6 +22,7 @@ test_expect_success setup '
> git branch side1 &&
> git branch side2 &&
> git branch side3 &&
> + git branch side4 &&
>
> git checkout side1 &&
> test_write_lines 1 2 3 4 5 6 >numbers &&
> @@ -46,6 +47,13 @@ test_expect_success setup '
> test_tick &&
> git commit -m rename-numbers &&
>
> + git checkout side4 &&
> + test_write_lines 0 1 2 3 4 5 >numbers &&
> + echo yo >greeting &&
> + git add numbers greeting &&
> + test_tick &&
> + git commit -m other-content-modifications &&
> +
> git switch --orphan unrelated &&
> >something-else &&
> git add something-else &&
> @@ -97,6 +105,21 @@ test_expect_success 'Content merge and a few conflicts' '
> test_cmp expect actual
> '
>
> +test_expect_success 'Auto resolve conflicts by "ours" strategy option' '
> + git checkout side1^0 &&
> +
> + # make sure merge conflict exists
> + test_must_fail git merge side4 &&
> + git merge --abort &&
> +
> + git merge -X ours side4 &&
> + git rev-parse HEAD^{tree} > expected &&
> +
> + git merge-tree -X ours side1 side4 > actual &&
> +
> + test_cmp expected actual
> +'
> +
> test_expect_success 'Barf on misspelled option, with exit code other than 0 or 1' '
> # Mis-spell with single "s" instead of double "s"
> test_expect_code 129 git merge-tree --write-tree --mesages FOOBAR side1 side2 2>expect &&
>
> base-commit: ac83bc5054c2ac489166072334b4147ce6d0fccb
> --
> gitgitgadget
^ permalink raw reply
* Re: [PATCH] doc: pull: improve rebase=false documentation
From: Dragan Simic @ 2023-09-16 2:18 UTC (permalink / raw)
To: Linus Arver; +Cc: Junio C Hamano, git
In-Reply-To: <owlya5tnjg4f.fsf@fine.c.googlers.com>
On 2023-09-15 23:12, Linus Arver wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Linus Arver <linusa@google.com> writes:
>>
>>> Aside: interestingly, there appears to be a "--no-rebase" option that
>>> means "--rebase=false" (see cd67e4d46b (Teach 'git pull' about
>>> --rebase,
>>> 2007-11-28)):
>>>
>>> --no-rebase
>>> This is shorthand for --rebase=false.
>>> ...
>>> How about adding something like this instead as the very first
>>> paragraph
>>> for this flag?
>>>
>>> Supplying "--rebase" defaults to "--rebase=true". Running
>>> git-pull
>>> without arguments implies "--rebase=false", unless relevant
>>> configuration variables have been set otherwise.
>>
>> Phrase nit.
>>
>> $ git pull origin
>>
>> does run the command with arguments.
>
> Ah, good catch.
>
>> What you mean is "running
>> git-pull without any --rebase arguments implies --no-rebase",
>
> Right (modulo your "--rebase arguments" -> "--rebase option" correction
> in your follow-up email).
>
>> but
>> that is saying "not giving --rebase=<any> and not giving --rebase
>> means not rebasing", which makes my head spin.
>
> Me too.
>
>> "--no-rebase" as a command line option does have use to defeat
>> configured pull.rebase that is not set to "false"
>
> Yes, I noticed this too after digging around a bit more after I sent my
> message. Thanks for clarifying for the record.
>
>> and allowing
>> "pull.rebase" to be set to "false" does have use to defeat settings
>> for the same variable made by lower-precedence configuration file.
>
> Indeed! I did not think of this. IOW, Git can be configured in
> multiple places (the "pull.rebase" setting in ~/.gitconfig can be
> overridden by the same config in ~/myrepo/.git/config).
>
>> "--rebase=false" does not have any reason to exist, except for
>> making the repertoire of "--rebase=<kind>" to be complete.
>
> Agreed. In a sense, the docs for "--rebase=false" should say that it is
> a synonym for "--no-rebase" (because "--no-rebase" came first,
> historically), and not the other way around (that "--no-rebase" is
> shorthand for "--rebase=false").
>
>> So, I am still not sure if saying "'git pull' (no other arguments
>> and no configuration) is equivalent to 'git pull --rebase=false'"
>> adds much value.
>
> Fair point. That is, there are so many gotchas and "edge case"-like
> behaviors to consider here, so the statement "'git pull' (no other
> arguments
> and no configuration) is equivalent to 'git pull --rebase=false'" is an
> oversimplification that can be misleading. I agree.
>
>> If --no-rebase and --rebase=false are explained in terms of why
>> these options that specify such an unnatural action (after all, you
>> say "do this" or "do it this way", but do not usually have to say
>> "do not do it that way") need to exist.
>>
>> If I were writing this patch, I would rearrange the existing text
>> like so:
>>
>> * Update the description of "--no-rebase" *NOT* to depend on
>> --rebase=false. Instead move it higher and say
>>
>> - The default for "git pull" is to "merge" the other history into
>> your history, but optionally you can "rebase" your history on
>> top of the other history.
>>
>> - There are configuration variables (pull.rebase and
>> branch.<name>.rebase) that trigger the optional behaviour, and
>> when you set it, your "git pull" would "rebase".
>>
>> - The "--no-rebase" option is to defeat such configuration to
>> tell the command to "merge" for this particular invocation.
>>
>> * Update the description of "--rebase=<kind>" and move the
>> paragraph that begins with "When false" to the end, something
>> like:
>>
>> - `--rebase` alone is equivalent to `--rebase=true`.
>> - When set to 'merges'...
>> - When set to 'interactive'...
>> - See `pull.rebase`, ..., if you want to make `git pull` always
>> rebase your history on top of theirs, instead of merging their
>> history to yours.
>> - `--rebase=false` is synonym to `--no-rebase`.
>
> I think this captures the finer details while still preserving the
> spirit of Dragan's original patch, so SGTM.
>
> @Dragan if you are OK with doing the (much more substantial) change as
> suggested, please do. Thanks!
Sure, thanks. I'll put together v2 of the patch in the next couple of
days, taking into account all the fine details of the awesome feedback
provided by both of you, so we can have it discussed and refined
further.
^ permalink raw reply
* [PATCH v3] merge-tree: add -X strategy option
From: Izzy via GitGitGadget @ 2023-09-16 2:14 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Izzy, Tang Yuyi
In-Reply-To: <pull.1565.v2.git.1691818386345.gitgitgadget@gmail.com>
From: Tang Yuyi <winglovet@gmail.com>
Add merge strategy option to produce more customizable merge result such
as automatically solve conflicts.
Signed-off-by: Tang Yuyi <winglovet@gmail.com>
---
merge-tree: add -X strategy option
Change-Id: I16be592262d13cebcff8726eb043f7ecdb313b76
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1565%2FWingT%2Fmerge_tree_allow_strategy_option-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1565
Range-diff vs v2:
1: 7d53d08381e ! 1: d64a774fa7c merge-tree: add -X strategy option
@@
## Metadata ##
-Author: winglovet <winglovet@gmail.com>
+Author: Tang Yuyi <winglovet@gmail.com>
## Commit message ##
merge-tree: add -X strategy option
@@ Commit message
Add merge strategy option to produce more customizable merge result such
as automatically solve conflicts.
- Signed-off-by: winglovet <winglovet@gmail.com>
+ Signed-off-by: Tang Yuyi <winglovet@gmail.com>
## builtin/merge-tree.c ##
@@
@@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Content merge and a few c
test_cmp expect actual
'
-+test_expect_success 'Auto resolve conflicts by "ours" stragety option' '
++test_expect_success 'Auto resolve conflicts by "ours" strategy option' '
+ git checkout side1^0 &&
+
+ # make sure merge conflict exists
builtin/merge-tree.c | 24 ++++++++++++++++++++++++
t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 0de42aecf4b..2ec6ec0d39a 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -19,6 +19,8 @@
#include "tree.h"
#include "config.h"
+static const char **xopts;
+static size_t xopts_nr, xopts_alloc;
static int line_termination = '\n';
struct merge_list {
@@ -414,6 +416,7 @@ struct merge_tree_options {
int show_messages;
int name_only;
int use_stdin;
+ struct merge_options merge_options;
};
static int real_merge(struct merge_tree_options *o,
@@ -439,6 +442,8 @@ static int real_merge(struct merge_tree_options *o,
init_merge_options(&opt, the_repository);
+ opt.recursive_variant = o->merge_options.recursive_variant;
+
opt.show_rename_progress = 0;
opt.branch1 = branch1;
@@ -510,6 +515,17 @@ static int real_merge(struct merge_tree_options *o,
return !result.clean; /* result.clean < 0 handled above */
}
+static int option_parse_x(const struct option *opt,
+ const char *arg, int unset)
+{
+ if (unset)
+ return 0;
+
+ ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
+ xopts[xopts_nr++] = xstrdup(arg);
+ return 0;
+}
+
int cmd_merge_tree(int argc, const char **argv, const char *prefix)
{
struct merge_tree_options o = { .show_messages = -1 };
@@ -548,6 +564,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
&merge_base,
N_("commit"),
N_("specify a merge-base for the merge")),
+ OPT_CALLBACK('X', "strategy-option", &xopts,
+ N_("option=value"),
+ N_("option for selected merge strategy"),
+ option_parse_x),
OPT_END()
};
@@ -556,6 +576,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, mt_options,
merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+ for (int x = 0; x < xopts_nr; x++)
+ if (parse_merge_opt(&o.merge_options, xopts[x]))
+ die(_("unknown strategy option: -X%s"), xopts[x]);
+
/* Handle --stdin */
if (o.use_stdin) {
struct strbuf buf = STRBUF_INIT;
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 250f721795b..4125bb101ec 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -22,6 +22,7 @@ test_expect_success setup '
git branch side1 &&
git branch side2 &&
git branch side3 &&
+ git branch side4 &&
git checkout side1 &&
test_write_lines 1 2 3 4 5 6 >numbers &&
@@ -46,6 +47,13 @@ test_expect_success setup '
test_tick &&
git commit -m rename-numbers &&
+ git checkout side4 &&
+ test_write_lines 0 1 2 3 4 5 >numbers &&
+ echo yo >greeting &&
+ git add numbers greeting &&
+ test_tick &&
+ git commit -m other-content-modifications &&
+
git switch --orphan unrelated &&
>something-else &&
git add something-else &&
@@ -97,6 +105,21 @@ test_expect_success 'Content merge and a few conflicts' '
test_cmp expect actual
'
+test_expect_success 'Auto resolve conflicts by "ours" strategy option' '
+ git checkout side1^0 &&
+
+ # make sure merge conflict exists
+ test_must_fail git merge side4 &&
+ git merge --abort &&
+
+ git merge -X ours side4 &&
+ git rev-parse HEAD^{tree} > expected &&
+
+ git merge-tree -X ours side1 side4 > actual &&
+
+ test_cmp expected actual
+'
+
test_expect_success 'Barf on misspelled option, with exit code other than 0 or 1' '
# Mis-spell with single "s" instead of double "s"
test_expect_code 129 git merge-tree --write-tree --mesages FOOBAR side1 side2 2>expect &&
base-commit: ac83bc5054c2ac489166072334b4147ce6d0fccb
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] diff --stat: add config option to limit filename width
From: Dragan Simic @ 2023-09-16 2:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <487bd30e5a4cdcea8697393eb36ce3f3@manjaro.org>
On 2023-09-12 04:11, Dragan Simic wrote:
> On 2023-09-12 01:12, Junio C Hamano wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>>
>>> Add new configuration option diff.statNameWidth=<width> that is
>>> equivalent
>>> to the command-line option --stat-name-width=<width>, but it is
>>> ignored
>>> by format-patch. This follows the logic established by the already
>>> existing configuration option diff.statGraphWidth=<width>.
>>>
>>> Limiting the widths of names and graphs in the --stat output makes
>>> sense
>>> for interactive work on wide terminals with many columns, hence the
>>> support
>>> for these configuration options. They don't affect format-patch
>>> because
>>> it already adheres to the traditional 80-column standard.
>>>
>>> Update the documentation and add more tests to cover new
>>> configuration
>>> option diff.statNameWidth=<width>. While there, perform a few minor
>>> code
>>> and whitespace cleanups here and there, as spotted.
>>>
>>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>>> ---
>>
>> The stat lines have <width> (the entire display width),
>> <graph-width> (what appears after '|') and <name-width> (what
>> appears before '|'), so I would worry about letting users specify
>> all three to contradictory values, if there weren't an existing
>> command line option already. But of course there already is a
>> command line option, so somebody more clever than me must have
>> thought about how to deal with such an impossible settings, and
>> adding a configuration variable to specify the same impossible
>> settings to the system would not make things worse.
>
> Good point, but we're actually fine with adding diff.statNameWidth as
> a new configuration option, because the real troubles with
> contradictory configuration values might arise if we ever add
> diff.statWidth later. However, we should still be fine at that point,
> because the code in diff.c, starting around the line #2730, performs a
> reasonable amount of sanity checks and value adjustments.
>
> If we ever add diff.statWidth later, a good thing to do would be to
> emit warnings from the above-mentioned code in diff.c if it actually
> performs the adjustments, to make the users aware of the contradictory
> values. I might even have a look at that separately, if you're fine
> with adding such warnings.
Just checking, do you want me to perform any improvements to this patch,
so you can have it pulled into one of your trees?
I'll start working on a patch that adds the above-mentioned warnings,
but having those implemented properly and hashed out will surely take a
fair amount of time. However, those warnings should be quite usable, if
you agree, although they're not critical at the moment.
>>> Documentation/config/diff.txt | 4 ++++
>>> Documentation/diff-options.txt | 17 +++++++-------
>>> builtin/diff.c | 1 +
>>> builtin/log.c | 1 +
>>> builtin/merge.c | 1 +
>>> builtin/rebase.c | 1 +
>>
>> Someday, as a follow-up after the dust from this topic settles, we
>> would probably want to look at how these rev.diffopt.* members are
>> initialized and refactor the common code out to a helper. It would
>> allow us to instead of doing this ...
>
> Another good point. If you agree, I'd prefer to have my patch
> accepted and merged as-is, after which I'll have a look into unifying
> the initialization of the rev.diffopt.* members. Such an approach
> should, in general, also be better in case any regressions are
> detected at some point in the future.
>
> I'll also have a look into the NEEDSWORK note in diff.c that asks for
> using utf8_strnwidth() to calculate the display width of line_prefix.
> I already had a brief look at that, and it seems that it leaves enough
> space for some rather nice related code cleanups.
I'll also start working on these patches in the next few days, which
should result in some rather nice code cleanups, AFAICT so far.
>>> /* Set up defaults that will apply to both no-index and regular
>>> diffs. */
>>> rev.diffopt.stat_width = -1;
>>> + rev.diffopt.stat_name_width = -1;
>>> rev.diffopt.stat_graph_width = -1;
>>> rev.diffopt.flags.allow_external = 1;
>>> rev.diffopt.flags.allow_textconv = 1;
>>
>> ... in many places, do so in a single place in the helper function,
>> and these places will just call the helper:
>>
>> std_graph_options(&rev.diffopt);
>>
>> I do not know offhand if "stat graph options related members" is a
>> good line to draw, or there are other things that are common outside
>> these .stat_foo members. If the latter and the helper function will
>> initialize the members other than the stat-graph settings, its name
>> obviously needs a bit more thought, but you get the idae.
>
> Sure, I'm willing to have a detailed look into all that, as I already
> described above.
^ permalink raw reply
* What's cooking in git.git (Sep 2023, #05; Fri, 15)
From: Junio C Hamano @ 2023-09-16 1:41 UTC (permalink / raw)
To: git
Here are the topics that have been cooking in my tree. Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a
future release). Commits prefixed with '-' are only in 'seen', and
aren't considered "accepted" at all and may be annotated with an URL
to a message that raises issues but they are no means exhaustive. A
topic without enough support may be discarded after a long period of
no activity (of course they can be resubmit when new interests
arise).
Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors. Some
repositories have only a subset of branches.
With maint, master, next, seen, todo:
git://git.kernel.org/pub/scm/git/git.git/
git://repo.or.cz/alt-git.git/
https://kernel.googlesource.com/pub/scm/git/git/
https://github.com/git/git/
https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
https://github.com/gitster/git/
Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):
git://git.kernel.org/pub/scm/git/git-htmldocs.git/
https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
https://www.kernel.org/pub/software/scm/git/
--------------------------------------------------
[Graduated to 'master']
* ak/pretty-decorate-more (2023-08-21) 8 commits
(merged to 'next' on 2023-09-07 at 6f5e14cef2)
+ decorate: use commit color for HEAD arrow
+ pretty: add pointer and tag options to %(decorate)
+ pretty: add %(decorate[:<options>]) format
+ decorate: color each token separately
+ decorate: avoid some unnecessary color overhead
+ decorate: refactor format_decorations()
+ pretty-formats: enclose options in angle brackets
+ pretty-formats: define "literal formatting code"
"git log --format" has been taught the %(decorate) placeholder.
source: <20230820185009.20095-1-andy.koppe@gmail.com>
* bc/more-git-var (2023-09-05) 1 commit
(merged to 'next' on 2023-09-06 at d8af32874c)
+ var: avoid a segmentation fault when `HOME` is unset
Fix-up for a topic that already has graduated.
source: <pull.1580.git.1693808487058.gitgitgadget@gmail.com>
* ew/hash-with-openssl-evp (2023-08-31) 1 commit
(merged to 'next' on 2023-09-05 at 1ddc0078c8)
+ treewide: fix various bugs w/ OpenSSL 3+ EVP API
Fix-up new-ish code to support OpenSSL EVP API.
source: <20230901020928.M610756@dcvr>
* jk/tree-name-and-depth-limit (2023-08-31) 10 commits
(merged to 'next' on 2023-09-07 at 7ad767ab0d)
+ lower core.maxTreeDepth default to 2048
+ tree-diff: respect max_allowed_tree_depth
+ list-objects: respect max_allowed_tree_depth
+ read_tree(): respect max_allowed_tree_depth
+ traverse_trees(): respect max_allowed_tree_depth
+ add core.maxTreeDepth config
+ fsck: detect very large tree pathnames
+ tree-walk: rename "error" variable
+ tree-walk: drop MAX_TRAVERSE_TREES macro
+ tree-walk: reduce stack size for recursive functions
We now limit depth of the tree objects and maximum length of
pathnames recorded in tree objects.
source: <20230831061735.GA2702156@coredump.intra.peff.net>
* jk/unused-post-2.42-part2 (2023-09-05) 10 commits
(merged to 'next' on 2023-09-05 at 308ca3a052)
+ parse-options: mark unused parameters in noop callback
+ interpret-trailers: mark unused "unset" parameters in option callbacks
+ parse-options: add more BUG_ON() annotations
+ merge: do not pass unused opt->value parameter
+ parse-options: mark unused "opt" parameter in callbacks
+ parse-options: prefer opt->value to globals in callbacks
+ checkout-index: delay automatic setting of to_tempfile
+ format-patch: use OPT_STRING_LIST for to/cc options
+ merge: simplify parsing of "-n" option
+ merge: make xopts a strvec
Unused parameters to functions are marked as such, and/or removed,
in order to bring us closer to -Wunused-parameter clean.
source: <20230831211637.GA949188@coredump.intra.peff.net>
* ks/ref-filter-sort-numerically (2023-09-05) 1 commit
(merged to 'next' on 2023-09-06 at aa4d156366)
+ ref-filter: sort numerically when ":size" is used
"git for-each-ref --sort='contents:size'" sorts the refs according
to size numerically, giving a ref that points at a blob twelve-byte
(12) long before showing a blob hundred-byte (100) long.
source: <20230902090155.8978-1-five231003@gmail.com>
* ob/revert-of-revert-is-reapply (2023-09-02) 2 commits
(merged to 'next' on 2023-09-07 at 9a54f66511)
+ git-revert.txt: add discussion
+ sequencer: beautify subject of reverts of reverts
The default log message created by "git revert", when reverting a
commit that records a revert, has been tweaked.
source: <20230821170720.577850-1-oswald.buddenhagen@gmx.de>
source: <20230902072035.652549-1-oswald.buddenhagen@gmx.de>
* ob/sequencer-reword-error-message (2023-09-05) 1 commit
(merged to 'next' on 2023-09-06 at c5154b7aa2)
+ sequencer: fix error message on failure to copy SQUASH_MSG
Update an error message (which would probably never been seen).
source: <20230903151132.739136-1-oswald.buddenhagen@gmx.de>
* pw/rebase-i-after-failure (2023-09-06) 7 commits
(merged to 'next' on 2023-09-07 at 3cbc3c4d63)
+ rebase -i: fix adding failed command to the todo list
+ rebase --continue: refuse to commit after failed command
+ rebase: fix rewritten list for failed pick
+ sequencer: factor out part of pick_commits()
+ sequencer: use rebase_path_message()
+ rebase -i: remove patch file after conflict resolution
+ rebase -i: move unlink() calls
Various fixes to the behaviour of "rebase -i" when the command got
interrupted by conflicting changes.
cf. <6b927687-cf6e-d73e-78fb-bd4f46736928@gmx.de>
source: <pull.1492.v4.git.1694013771.gitgitgadget@gmail.com>
* rs/grep-parseopt-simplify (2023-09-05) 1 commit
(merged to 'next' on 2023-09-06 at 249b69cfd2)
+ grep: use OPT_INTEGER_F for --max-depth
Simplify use of parse-options API a bit.
source: <4d2eb736-4f34-18f8-2eb7-20e7f7b8c2f8@web.de>
* rs/name-rev-use-opt-hidden-bool (2023-09-05) 1 commit
(merged to 'next' on 2023-09-06 at 9b251a5392)
+ name-rev: use OPT_HIDDEN_BOOL for --peel-tag
Simplify use of parse-options API a bit.
source: <5a86c8f8-fcdc-fee9-8af5-aa5ecb036d2e@web.de>
* so/diff-doc-for-patch-update (2023-09-06) 1 commit
(merged to 'next' on 2023-09-07 at 6da5e9defd)
+ doc/diff-options: fix link to generating patch section
References from description of the `--patch` option in various
manual pages have been simplified and improved.
source: <87msxzpybo.fsf_-_@osv.gnss.ru>
--------------------------------------------------
[New Topics]
* ob/sequencer-remove-dead-code (2023-09-12) 1 commit
(merged to 'next' on 2023-09-13 at 1f6c2b336b)
+ sequencer: remove unreachable exit condition in pick_commits()
Code clean-up.
Will merge to 'master'.
source: <20230912105541.272917-1-oswald.buddenhagen@gmx.de>
* ob/t3404-typofix (2023-09-12) 1 commit
(merged to 'next' on 2023-09-13 at b17fa78c3f)
+ t3404-rebase-interactive.sh: fix typos in title of a rewording test
Code clean-up.
Will merge to 'master'.
source: <20230912104237.271616-1-oswald.buddenhagen@gmx.de>
* jk/ort-unused-parameter-cleanups (2023-09-14) 4 commits
- merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()
- merge-ort: drop unused parameters from detect_and_process_renames()
- merge-ort: stop passing "opt" to read_oid_strbuf()
- merge-ort: drop custom err() function
Code clean-up.
Needs review.
source: <20230914093409.GA2254811@coredump.intra.peff.net>
* jc/fake-lstat (2023-09-14) 1 commit
- cache: add fake_lstat()
A new helper to let us pretend that we called lstat() when we know
our cache_entry is up-to-date via fsmonitor.
Needs review.
source: <xmqqcyykig1l.fsf@gitster.g>
* jk/redact-h2h3-headers-fix (2023-09-15) 2 commits
- http: update curl http/2 info matching for curl 8.3.0
- http: factor out matching of curl http/2 trace lines
HTTP Header redaction code has been adjusted for a newer version of
cURL library that shows its traces differently from earlier
versions.
Will merge to 'next'.
source: <20230915113237.GA3531328@coredump.intra.peff.net>
* ch/clean-docfix (2023-09-15) 1 commit
- git-clean doc: fix "without do cleaning" typo
Typofix.
Will merge to 'next'.
source: <pull.1572.git.git.1694800409471.gitgitgadget@gmail.com>
* eg/config-type-path-docfix (2023-09-15) 1 commit
- git-config: fix misworded --type=path explanation
Typofix.
Will merge to 'next'.
source: <20230915202610.21206-1-evan.gates@gmail.com>
* kn/rev-list-ignore-missing-links (2023-09-15) 1 commit
- revision: add `--ignore-missing-links` user option
Surface the .ignore_missing_links bit that stops the revision
traversal from stopping and dying when encountering a missing
object to a new command line option of "git rev-list", so that the
objects that are required but are missing can be enumerated.
Waiting for review response.
source: <20230915083415.263187-1-knayak@gitlab.com>
--------------------------------------------------
[Stalled]
* tk/cherry-pick-sequence-requires-clean-worktree (2023-06-01) 1 commit
- cherry-pick: refuse cherry-pick sequence if index is dirty
"git cherry-pick A" that replays a single commit stopped before
clobbering local modification, but "git cherry-pick A..B" did not,
which has been corrected.
Expecting a reroll.
cf. <999f12b2-38d6-f446-e763-4985116ad37d@gmail.com>
source: <pull.1535.v2.git.1685264889088.gitgitgadget@gmail.com>
--------------------------------------------------
[Cooking]
* js/systemd-timers-wsl-fix (2023-09-11) 1 commit
(merged to 'next' on 2023-09-12 at 71c4cbb1df)
+ maintenance(systemd): support the Windows Subsystem for Linux
Update "git maintainance" timers' implementation based on systemd
timers to work with WSL.
Will merge to 'master'.
source: <pull.1586.git.1694334620260.gitgitgadget@gmail.com>
* kh/range-diff-notes (2023-09-11) 1 commit
- range-diff: treat notes like `log`
"git range-diff --notes=foo" compared "log --notes=foo --notes" of
the two ranges, instead of using just the specified notes tree.
Expecting a reroll.
cf. <dd2958c5-58bf-86dd-b666-9033259a8e1a@gmx.de>
source: <a37dfb3748e23b4f5081bc9a3c80a5c546101f1d.1694383248.git.code@khaugsbakk.name>
* pw/diff-no-index-from-named-pipes (2023-09-11) 1 commit
(merged to 'next' on 2023-09-12 at 135ecd136f)
+ diff --no-index: fix -R with stdin
"git diff --no-index -R <(one) <(two)" did not work correctly,
which has been corrected.
Will merge to 'master'.
source: <22fdfa3b-f90e-afcc-667c-705fb7670245@web.de>
* rs/parse-options-value-int (2023-09-11) 2 commits
- parse-options: use and require int pointer for OPT_CMDMODE
- parse-options: add int value pointer to struct option
A bit of type safety for the "value" pointer used in the
parse-options API.
What's the status of this one?
source: <e6d8a291-03de-cfd3-3813-747fc2cad145@web.de>
* so/diff-merges-d (2023-09-11) 2 commits
- diff-merges: introduce '-d' option
- diff-merges: improve --diff-merges documentation
Teach a new "-d" option that shows the patch against the first
parent for merge commits (which is "--diff-merges=first-parent -p").
Needs more work.
source: <20230909125446.142715-1-sorganov@gmail.com>
* js/diff-cached-fsmonitor-fix (2023-09-11) 1 commit
(merged to 'next' on 2023-09-12 at 7479278da0)
+ diff-lib: fix check_removed when fsmonitor is on
"git diff --cached" codepath did not fill the necessary stat
information for a file when fsmonitor knows it is clean and ended
up behaving as if it is not clean, which has been corrected.
Will merge to 'master'.
source: <20230911170901.49050-2-sokcevic@google.com>
* pb/completion-aliases-doc (2023-09-12) 1 commit
(merged to 'next' on 2023-09-13 at b248a5bc26)
+ completion: improve doc for complex aliases
Clarify how "alias.foo = : git cmd ; aliased-command-string" should
be spelled with necessary whitespaces around punctuation marks to
work.
Will merge to 'master'.
source: <pull.1585.v2.git.1694538135853.gitgitgadget@gmail.com>
* cc/repack-sift-filtered-objects-to-separate-pack (2023-09-11) 9 commits
. gc: add `gc.repackFilterTo` config option
. repack: implement `--filter-to` for storing filtered out objects
. gc: add `gc.repackFilter` config option
. repack: add `--filter=<filter-spec>` option
. pack-bitmap-write: rebuild using new bitmap when remapping
. repack: refactor finding pack prefix
. repack: refactor finishing pack-objects command
. t/helper: add 'find-pack' test-tool
. pack-objects: allow `--filter` without `--stdout`
"git repack" machinery learns to pay attention to the "--filter="
option.
May need to wait until tb/repack-existing-packs-cleanup stablizes.
source: <20230911150618.129737-1-christian.couder@gmail.com>
* la/trailer-cleanups (2023-09-11) 6 commits
(merged to 'next' on 2023-09-12 at 779c4a097a)
+ trailer: use offsets for trailer_start/trailer_end
+ trailer: rename *_DEFAULT enums to *_UNSPECIFIED
+ trailer: teach find_patch_start about --no-divider
+ trailer: split process_command_line_args into separate functions
+ trailer: split process_input_file into separate pieces
+ trailer: separate public from internal portion of trailer_iterator
Code clean-up.
Will merge to 'master'.
source: <pull.1563.v2.git.1694240177.gitgitgadget@gmail.com>
* tb/repack-existing-packs-cleanup (2023-09-13) 8 commits
(merged to 'next' on 2023-09-14 at bb8065e89c)
+ builtin/repack.c: extract common cruft pack loop
+ builtin/repack.c: avoid directly inspecting "util"
+ builtin/repack.c: store existing cruft packs separately
+ builtin/repack.c: extract `has_existing_non_kept_packs()`
+ builtin/repack.c: extract redundant pack cleanup for existing packs
+ builtin/repack.c: extract redundant pack cleanup for --geometric
+ builtin/repack.c: extract marking packs for deletion
+ builtin/repack.c: extract structure to store existing packs
The code to keep track of existing packs in the repository while
repacking has been refactored.
Will merge to 'master'.
source: <cover.1694632644.git.me@ttaylorr.com>
* pb/complete-commit-trailers (2023-09-12) 2 commits
(merged to 'next' on 2023-09-13 at 9a0ec17606)
+ completion: commit: complete trailers tokens more robustly
(merged to 'next' on 2023-09-08 at 842587016d)
+ completion: commit: complete configured trailer tokens
The command-line complation support (in contrib/) learned to
complete "git commit --trailer=" for possible trailer keys.
Will merge to 'master'.
source: <pull.1583.v2.git.1694539827.gitgitgadget@gmail.com>
* pw/rebase-sigint (2023-09-07) 1 commit
- rebase -i: ignore signals when forking subprocesses
If the commit log editor or other external programs (spawned via
"exec" insn in the todo list) receive internactive signal during
"git rebase -i", it caused not just the spawned program but the
"Git" process that spawned them, which is often not what the end
user intended. "git" learned to ignore SIGINT and SIGQUIT while
waiting for these subprocesses.
Expecting a reroll.
cf. <12c956ea-330d-4441-937f-7885ab519e26@gmail.com>
source: <pull.1581.git.1694080982621.gitgitgadget@gmail.com>
* rs/grep-no-no-or (2023-09-07) 1 commit
(merged to 'next' on 2023-09-08 at 33849032bc)
+ grep: reject --no-or
"git grep -e A --no-or -e B" is accepted, even though the negation
of "or" did not mean anything, which has been tightened.
Will merge to 'master'.
source: <6aeb0ebe-0fea-ccd3-089a-ee0b5b5baf10@web.de>
* js/complete-checkout-t (2023-09-08) 1 commit
(merged to 'next' on 2023-09-08 at 461bb28fbd)
+ completion(switch/checkout): treat --track and -t the same
The completion script (in contrib/) has been taught to treat the
"-t" option to "git checkout" and "git switch" just like the
"--track" option, to complete remote-tracking branches.
Will merge to 'master'.
source: <pull.1584.git.1694176123471.gitgitgadget@gmail.com>
* cw/git-std-lib (2023-09-11) 7 commits
- SQUASH???
- git-std-lib: add test file to call git-std-lib.a functions
- git-std-lib: introduce git standard library
- parse: create new library for parsing strings and env values
- config: correct bad boolean env value error message
- wrapper: remove dependency to Git-specific internal file
- hex-ll: split out functionality from hex
Another libification effort.
Needs more work.
cf. <xmqqy1hfrk6p.fsf@gitster.g>
cf. <20230915183927.1597414-1-jonathantanmy@google.com>
source: <20230908174134.1026823-1-calvinwan@google.com>
* cc/git-replay (2023-09-07) 15 commits
- replay: stop assuming replayed branches do not diverge
- replay: add --contained to rebase contained branches
- replay: add --advance or 'cherry-pick' mode
- replay: disallow revision specific options and pathspecs
- replay: use standard revision ranges
- replay: make it a minimal server side command
- replay: remove HEAD related sanity check
- replay: remove progress and info output
- replay: add an important FIXME comment about gpg signing
- replay: don't simplify history
- replay: introduce pick_regular_commit()
- replay: die() instead of failing assert()
- replay: start using parse_options API
- replay: introduce new builtin
- t6429: remove switching aspects of fast-rebase
Waiting for review response.
cf. <52277471-4ddd-b2e0-62ca-c2a5b59ae418@gmx.de>
cf. <58daa706-7efb-51dd-9061-202ef650b96a@gmx.de>
cf. <f0e75d47-c277-9fbb-7bcd-53e4e5686f3c@gmx.de>
May want to wait until tb/repack-existing-packs-cleanup stabilizes.
source: <20230907092521.733746-1-christian.couder@gmail.com>
* la/trailer-test-and-doc-updates (2023-09-07) 13 commits
- trailer doc: <token> is a <key> or <keyAlias>, not both
- trailer doc: separator within key suppresses default separator
- trailer doc: emphasize the effect of configuration variables
- trailer --unfold help: prefer "reformat" over "join"
- trailer --parse docs: add explanation for its usefulness
- trailer --only-input: prefer "configuration variables" over "rules"
- trailer --parse help: expose aliased options
- trailer --no-divider help: describe usual "---" meaning
- trailer: trailer location is a place, not an action
- trailer doc: narrow down scope of --where and related flags
- trailer: add tests to check defaulting behavior with --no-* flags
- trailer test description: this tests --where=after, not --where=before
- trailer tests: make test cases self-contained
Test coverage for trailers has been improved.
source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
* js/doc-unit-tests (2023-08-17) 3 commits
- ci: run unit tests in CI
- unit tests: add TAP unit test framework
- unit tests: Add a project plan document
(this branch is used by js/doc-unit-tests-with-cmake.)
Process to add some form of low-level unit tests has started.
Waiting for review response.
cf. <xmqq350hw6n7.fsf@gitster.g>
source: <cover.1692297001.git.steadmon@google.com>
* js/doc-unit-tests-with-cmake (2023-08-31) 4 commits
- artifacts-tar: when including `.dll` files, don't forget the unit-tests
- unit-tests: do show relative file paths
- unit-tests: do not mistake `.pdb` files for being executable
- cmake: also build unit tests
(this branch uses js/doc-unit-tests.)
Update the base topic to work with CMake builds.
Waiting for the base topic to settle.
source: <pull.1579.git.1693462532.gitgitgadget@gmail.com>
* tb/path-filter-fix (2023-08-30) 15 commits
- bloom: introduce `deinit_bloom_filters()`
- commit-graph: reuse existing Bloom filters where possible
- object.h: fix mis-aligned flag bits table
- commit-graph: drop unnecessary `graph_read_bloom_data_context`
- commit-graph.c: unconditionally load Bloom filters
- t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
- bloom: prepare to discard incompatible Bloom filters
- bloom: annotate filters with hash version
- commit-graph: new filter ver. that fixes murmur3
- repo-settings: introduce commitgraph.changedPathsVersion
- t4216: test changed path filters with high bit paths
- t/helper/test-read-graph: implement `bloom-filters` mode
- bloom.h: make `load_bloom_filter_from_graph()` public
- t/helper/test-read-graph.c: extract `dump_graph_info()`
- gitformat-commit-graph: describe version 2 of BDAT
The Bloom filter used for path limited history traversal was broken
on systems whose "char" is unsigned; update the implementation and
bump the format version to 2.
Needs more work.
cf. <20230830200218.GA5147@szeder.dev>
source: <cover.1693413637.git.jonathantanmy@google.com>
* js/config-parse (2023-08-23) 4 commits
- config-parse: split library out of config.[c|h]
- config.c: accept config_parse_options in git_config_from_stdin
- config: report config parse errors using cb
- config: split out config_parse_options
The parsing routines for the configuration files have been split
into a separate file.
Needs review.
source: <cover.1692827403.git.steadmon@google.com>
* jc/update-index-show-index-version (2023-09-12) 3 commits
(merged to 'next' on 2023-09-13 at b754554df8)
+ test-tool: retire "index-version"
+ update-index: add --show-index-version
+ update-index doc: v4 is OK with JGit and libgit2
"git update-index" learns "--show-index-version" to inspect
the index format version used by the on-disk index file.
Will merge to 'master'.
source: <20230912193235.776292-1-gitster@pobox.com>
* jc/rerere-cleanup (2023-08-25) 4 commits
- rerere: modernize use of empty strbuf
- rerere: try_merge() should use LL_MERGE_ERROR when it means an error
- rerere: fix comment on handle_file() helper
- rerere: simplify check_one_conflict() helper function
(this branch uses jc/unresolve-removal.)
Code clean-up.
Not ready to be reviewed yet.
source: <20230731224409.4181277-1-gitster@pobox.com>
* jc/unresolve-removal (2023-07-31) 7 commits
- checkout: allow "checkout -m path" to unmerge removed paths
- checkout/restore: add basic tests for --merge
- checkout/restore: refuse unmerging paths unless checking out of the index
- update-index: remove stale fallback code for "--unresolve"
- update-index: use unmerge_index_entry() to support removal
- resolve-undo: allow resurrecting conflicted state that resolved to deletion
- update-index: do not read HEAD and MERGE_HEAD unconditionally
(this branch is used by jc/rerere-cleanup.)
"checkout --merge -- path" and "update-index --unresolve path" did
not resurrect conflicted state that was resolved to remove path,
but now they do.
Needs review.
source: <20230731224409.4181277-1-gitster@pobox.com>
* rj/status-bisect-while-rebase (2023-08-01) 1 commit
- status: fix branch shown when not only bisecting
"git status" is taught to show both the branch being bisected and
being rebased when both are in effect at the same time.
Needs review.
cf. <xmqqtttia3vn.fsf@gitster.g>
source: <48745298-f12b-8efb-4e48-90d2c22a8349@gmail.com>
^ permalink raw reply
* [PATCH] git-gui - re-enable use of hook scripts
From: Mark Levedahl @ 2023-09-16 0:35 UTC (permalink / raw)
To: gitster, johannes.schindelin, me, git; +Cc: Mark Levedahl
In-Reply-To: <454d8b7b-96df-ec8f-2285-e022de66c66c@gmail.com>
Commit aae9560a introduced search in $PATH to find executables before
running them, avoiding an issue where on Windows a same named file in
the current directory can be executed in preference to anything on the
path. The updated search excludes files given with an absolute path (e.g.,
/bin/sh). However this change precludes operation of hook scripts as these
are named with a relative path (.git/hooks/$HOOK), while a search on $PATH
can succeed only for bare file names, not relative paths. Furthermore,
the current repository's .git/hooks directory is in general not listed
in PATH.
Fix this by changing the "absolute" check to a check for more than one
component in the pathname, thereby avoiding the PATH check for anything
given with a relative path as well. Bare "git" has one component, "/sh"
has two components, and .git/hooks/$HOOK has more than two, so relative
and absolute pathnames avoid the check.
Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
git-gui.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-gui.sh b/git-gui.sh
index 8bc8892..8603437 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {
set i $from_index
while {$i < [llength $command_line]} {
set cmd [lindex $command_line $i]
- if {[file pathtype $cmd] ne "absolute"} {
+ if {[llength [file split $cmd]] < 2} {
set fullpath [_which $cmd]
if {$fullpath eq ""} {
throw {NOT-FOUND} "$cmd not found in PATH"
--
2.41.0.99.19
^ permalink raw reply related
* Re: BUG: git-gui no longer executes hook scripts
From: Mark Levedahl @ 2023-09-15 23:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes.schindelin, me
In-Reply-To: <xmqq5y4bgxy1.fsf@gitster.g>
On 9/15/23 13:15, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Shouldn't this "is it absolute" check with "$cmd" also check if $cmd
>> has either forward or backward slash in it?
>>
>> Checking the use of _which with fixed arguments, it is used to spawn
>> git, gitk, nice, sh; and _which finding where they appear on the
>> search path does sound sane. But _which does not seem to have the "if
>> given a command with directory separator, the search path does not
>> matter. The caller means it is relative to the $cwd" logic at all,
>> so it seems it is the callers responsibility to make sure it does
>> not pass things like ".git/hooks/pre-commit" to it.
> In other words, something along this line may go in the right
> direction (I no longer speak Tcl, and this is done with manual in
> one hand, while typing with the other hand).
>
I think a simpler fix is just to examine the number of path components -
more than one means a relative or absolute command (/foo splits into two
parts). The below works for me on Linux.
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 277a2b1c8c..0c39d9fa26 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {
set i $from_index
while {$i < [llength $command_line]} {
set cmd [lindex $command_line $i]
- if {[file pathtype $cmd] ne "absolute"} {
+ if {[llength [file split $cmd]] < 2} {
set fullpath [_which $cmd]
if {$fullpath eq ""} {
throw {NOT-FOUND} "$cmd not found in PATH"
We could also wrap the entirety of commit aae9560a in
if {[is_Windows]} { ... }
as all of this code is fixing a Windows specific vulnerability, though a
fix like the above is needed regardless.
Mark
^ permalink raw reply related
* Re: [PATCH] docs: fix "without do cleaning" typo
From: Junio C Hamano @ 2023-09-15 23:17 UTC (permalink / raw)
To: Caleb Hill via GitGitGadget; +Cc: git, Caleb Hill, Caleb Hill
In-Reply-To: <pull.1572.git.git.1694800409471.gitgitgadget@gmail.com>
"Caleb Hill via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Caleb Hill <chill389cc@gmail.com>
>
> This pr fixes a typo I noticed today while reading documentation here:
> https://git-scm.com/docs/git-clean#Documentation/git-clean.txt-quit
>
> Signed-off-by: Caleb Hill <chill389cc@gmail.com>
> ---
> docs: fix "without do cleaning" typo
>
> This pr fixes a simple typo I noticed today while reading documentation
> here:
> https://git-scm.com/docs/git-clean#Documentation/git-clean.txt-quit
Thanks; I'll reword the proposed log message a bit and queue.
> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
> index 160d08b86bb..5e1a3d5148c 100644
> --- a/Documentation/git-clean.txt
> +++ b/Documentation/git-clean.txt
> @@ -127,7 +127,7 @@ ask each::
>
> quit::
>
> - This lets you quit without do cleaning.
> + This lets you quit without doing any cleaning.
"without cleaning" would probably mean the same thing and a tad
shorter, but let's use what you wrote as-is.
Thanks.
^ permalink raw reply
* Re: [PATCH] doc: pull: improve rebase=false documentation
From: Linus Arver @ 2023-09-15 21:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Dragan Simic, git
In-Reply-To: <xmqqh6nvfi2p.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> Aside: interestingly, there appears to be a "--no-rebase" option that
>> means "--rebase=false" (see cd67e4d46b (Teach 'git pull' about --rebase,
>> 2007-11-28)):
>>
>> --no-rebase
>> This is shorthand for --rebase=false.
>> ...
>> How about adding something like this instead as the very first paragraph
>> for this flag?
>>
>> Supplying "--rebase" defaults to "--rebase=true". Running git-pull
>> without arguments implies "--rebase=false", unless relevant
>> configuration variables have been set otherwise.
>
> Phrase nit.
>
> $ git pull origin
>
> does run the command with arguments.
Ah, good catch.
> What you mean is "running
> git-pull without any --rebase arguments implies --no-rebase",
Right (modulo your "--rebase arguments" -> "--rebase option" correction
in your follow-up email).
> but
> that is saying "not giving --rebase=<any> and not giving --rebase
> means not rebasing", which makes my head spin.
Me too.
> "--no-rebase" as a command line option does have use to defeat
> configured pull.rebase that is not set to "false"
Yes, I noticed this too after digging around a bit more after I sent my
message. Thanks for clarifying for the record.
> and allowing
> "pull.rebase" to be set to "false" does have use to defeat settings
> for the same variable made by lower-precedence configuration file.
Indeed! I did not think of this. IOW, Git can be configured in
multiple places (the "pull.rebase" setting in ~/.gitconfig can be
overridden by the same config in ~/myrepo/.git/config).
> "--rebase=false" does not have any reason to exist, except for
> making the repertoire of "--rebase=<kind>" to be complete.
Agreed. In a sense, the docs for "--rebase=false" should say that it is
a synonym for "--no-rebase" (because "--no-rebase" came first,
historically), and not the other way around (that "--no-rebase" is
shorthand for "--rebase=false").
> So, I am still not sure if saying "'git pull' (no other arguments
> and no configuration) is equivalent to 'git pull --rebase=false'"
> adds much value.
Fair point. That is, there are so many gotchas and "edge case"-like
behaviors to consider here, so the statement "'git pull' (no other arguments
and no configuration) is equivalent to 'git pull --rebase=false'" is an
oversimplification that can be misleading. I agree.
> If --no-rebase and --rebase=false are explained in terms of why
> these options that specify such an unnatural action (after all, you
> say "do this" or "do it this way", but do not usually have to say
> "do not do it that way") need to exist.
>
> If I were writing this patch, I would rearrange the existing text
> like so:
>
> * Update the description of "--no-rebase" *NOT* to depend on
> --rebase=false. Instead move it higher and say
>
> - The default for "git pull" is to "merge" the other history into
> your history, but optionally you can "rebase" your history on
> top of the other history.
>
> - There are configuration variables (pull.rebase and
> branch.<name>.rebase) that trigger the optional behaviour, and
> when you set it, your "git pull" would "rebase".
>
> - The "--no-rebase" option is to defeat such configuration to
> tell the command to "merge" for this particular invocation.
>
> * Update the description of "--rebase=<kind>" and move the
> paragraph that begins with "When false" to the end, something
> like:
>
> - `--rebase` alone is equivalent to `--rebase=true`.
> - When set to 'merges'...
> - When set to 'interactive'...
> - See `pull.rebase`, ..., if you want to make `git pull` always
> rebase your history on top of theirs, instead of merging their
> history to yours.
> - `--rebase=false` is synonym to `--no-rebase`.
I think this captures the finer details while still preserving the
spirit of Dragan's original patch, so SGTM.
@Dragan if you are OK with doing the (much more substantial) change as
suggested, please do. Thanks!
^ permalink raw reply
* Re: [RFC PATCH] Not computing changed path filter for root commits
From: SZEDER Gábor @ 2023-09-15 20:29 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, me, derrickstolee
In-Reply-To: <20230911223157.446269-1-jonathantanmy@google.com>
On Mon, Sep 11, 2023 at 03:31:56PM -0700, Jonathan Tan wrote:
> SZEDER Gábor suggested [2] that we change the revision walk to read
> changed path filters also for root commits, but I don't think that's
> possible - we have to tie reading changed path filters to when we read
> trees, and right now, we don't seem to read trees when evaluating root
> commits (rev_compare_tree() in revision.c is in the only code path that
> uses changed path filters, and it itself is only called per-parent and
> thus not called for root commits).
When encountering a root commit during a pathspec-limited revision
walk we call rev_same_tree_as_empty() instead of rev_compare_tree().
All that's missing there is checking the Bloom filter and accounting
for false positives.
^ permalink raw reply
* [PATCH] git-config: fix misworded --type=path explanation
From: Evan Gates @ 2023-09-15 20:24 UTC (permalink / raw)
To: git
When `--type=<type>` was added as a prefered alias for `--<type>`
the explanation for the path type was reworded. Whereas the previous
explanation said "expand a leading `~`" this was changed to "adding a
leading `~`". Change "adding" to "expanding" to correctly explain the
canonicalization.
Fixes: fb0dc3bac1 (builtin/config.c: support `--type=<type>` as preferred alias for `--<type>`)
Signed-off-by: Evan Gates <evan.gates@gmail.com>
---
Turns out --type=path is exactly what I had been looking for, but
it took some experimentation as I found the documentation confusing.
In hindsight it's obvious, but I hope this simple fix helps someone else
figure it out faster in the future.
Documentation/git-config.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 7a2bcb2f6c..b1caac887a 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -201,7 +201,7 @@ Valid `<type>`'s include:
1073741824 upon input.
- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
above.
-- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+- 'path': canonicalize by expanding a leading `~` to the value of `$HOME` and
`~user` to the home directory for the specified user. This specifier has no
effect when setting the value (but you can use `git config section.variable
~/` from the command line to let your shell do the expansion.)
--
2.42.0
^ permalink raw reply related
* Re: [PATCH v3 6/6] git-std-lib: add test file to call git-std-lib.a functions
From: Junio C Hamano @ 2023-09-15 20:22 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Calvin Wan, git, nasamuffin, linusa, phillip.wood123, vdye
In-Reply-To: <20230915184321.1598611-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> Calvin Wan <calvinwan@google.com> writes:
>> Add test file that directly or indirectly calls all functions defined in
>> git-std-lib.a object files to showcase that they do not reference
>> missing objects and that git-std-lib.a can stand on its own.
>>
>> Certain functions that cause the program to exit or are already called
>> by other functions are commented out.
>>
>> TODO: replace with unit tests
>> Signed-off-by: Calvin Wan <calvinwan@google.com>
>
> I think the TODO should go into the code, so that when we add a unit
> test that also deletes stdlib-test.c, we can see what's happening just
> from the diff. The TODO should also explain what stdlib-test.c is hoping
> to do, and why replacing it is OK. (Also, do we need to invoke all the
> functions? I thought that missing functions are checked at link time, or
> at the very latest, when the executable is run. No need to change this,
> though - invoking all the functions we can is fine.)
>
Thanks for excellent reviews (not just against this 6/6 but others,
too).
^ 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