* Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Junio C Hamano @ 2023-09-02 22:36 UTC (permalink / raw)
To: Wesley; +Cc: phillip.wood, git
In-Reply-To: <fa702b47-ae29-4299-9226-4920620b9fff@opperschaap.net>
Wesley <wesleys@opperschaap.net> writes:
> On 9/1/23 14:10, Junio C Hamano wrote:
>> Wesley <wesleys@opperschaap.net> writes:
>>
>>> The quirk is this: --fork-point looks at the reflog and reflog is
>>> local. Meaning, having an remote upstream branch will make
>>> --fork-point a noop. Only where you have an upstream which is local
>>> and your reflog has seen dropped commits it does something.
>> Why do you lack reflog on your remote-tracking branches in the first
>> place?
>
> I do not know? I tested with a bare repo and two clones. And I also
> tested it with just a remote upstream in another branch.
IIRC, a non-bare repository (i.e. with working tree) should get
core.logallrefupdates set to true by default, so all your refs, not
just local and remote-tracking branches, should have records.
> I haven't force pushed anything btw, maybe that could explain things?
If your "remote" is never force-pushed, then the movements of refs
at the remote (which you will observe whenever you fetch from it)
will always fast-forward, and the remote-tracking branches in your
local repository that keeps track of the movement will also record
the fast-forwarding movement in the reflog. But then there is no
need for the fork-point heurisitics to trigger, and even if it
triggered the heuristics would not change the outcome, when rebasing
against such a remote branch, as their tip will always a decendant
of all commits that ever sat at the tip of that remote branch.
^ permalink raw reply
* Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Junio C Hamano @ 2023-09-02 23:37 UTC (permalink / raw)
To: Wesley Schwengle; +Cc: git
In-Reply-To: <20230902221641.1399624-3-wesleys@opperschaap.net>
Wesley Schwengle <wesleys@opperschaap.net> writes:
> Subject: Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint
(applies to all three patches) downcase "Emit" after the <area>: prefix.
> This patch adds a warning where it will indicate that `rebase.forkpoint'
> must be set in the git configuration and/or that you can supply a
> `--fork-point' or `--no-fork-point' command line option to your `git
> rebase' invocation.
>
> When commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,
> 2021-09-16) was submitted there was a discussion on if the forkpoint
> behaviour of `git rebase' was sane. In my experience this wasn't sane.
I already said that the above is not true, so I will not repeat myself.
> Git rebase doesn't work if you don't have an upstream branch configured
"git rebase foo" works just fine, so this statement needs a lot of
tightening.
> (or something that says `merge = refs/heads/master' in the git config).
> You would than need to use `git rebase <upstream>' to rebase. If you
> configure an upstream it would seem logical to be able to run `git
> rebase' without arguments. However doing so would trigger a different
> kind of behavior. `git rebase <upstream>' behaves as if
> `--no-fork-point' was supplied and without it behaves as if
> `--fork-point' was supplied. This behavior can result in a loss of
> commits and can surprise users.
No, what is causing the loss in this particular case is allowing to
use the fork-point heuristics. If you do not want it, you can
either explicitly give --no-fork-point or <upstream> (or both if you
feel that you need to absolutely be clear). Or you can set the
configuration to "false" to disable this "auto" behaviour.
> The following reproduction path exposes
> this behavior:
I actually do not think having this example in the proposed log
message adds more value than it distracts readers from the real
point of this change.
If you rewind to lose commits from the branch you are (re)building
against, and what was rewound and discarded was part of the work you
are building, whether it is on a local branch or on a remote branch
that contains what you have already pushed, they will be discarded,
it is by design, and it is a known deficiency with the fork-point
heuristics. How the fork-point heuristics breaks down is rather
well known and it is pretty much orthogonal to the point of this
patch, which is to make it harder to trigger by folks who are not
familiar with "git rebase" and yet try to be lazy by not specifying
the <upstream> from the command line.
By the way, while I do agree with the need to make users _aware_ of
the "auto" behaviour [*1*], I am not yet convinced that there is a
need to change the default in the future.
Side note: It allows those who originally advocated the
fork-point heuristics to be extra lazy and allow fork-point
heuristics to be used when they rebuild on top of what they
usually rebuild on (and the "usually" part is signalled by
using "git rebase" without saying what to build on from the
command line). The default allows them not to worry about
the heuristics to kick in when they explicitly say on which
exact commit they want to rebuild on.
And when we do not know if the default will change, the new warning
message will lose value. Many of those who see the message are
already familiar with when the forkpoint heuristics will kick in,
and those who weren't familiar with will not know what the default
change is about, without consulting the documentation.
It might be better to extend the documentation instead, which will
not distract those who are using the tool just fine already.
> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
> index 4bfc779bb8..908867ae0f 100755
> --- a/t/t3431-rebase-fork-point.sh
> +++ b/t/t3431-rebase-fork-point.sh
> @@ -113,4 +113,66 @@ test_expect_success 'rebase.forkPoint set to true and --root given' '
> git rebase --root
> '
>
> +# The use of the diff -qw is because there is some kind of whitespace character
> +# magic going on which probably has to do with the tabs. It only occurs when we
> +# check STDERR
> +test_expect_success 'rebase without rebase.forkpoint' '
> + git init rebase-forkpoint &&
> + cd rebase-forkpoint &&
> + git status >/tmp/foo &&
> + echo "commit a" > file.txt &&
Style???
> + git add file.txt &&
> + git commit -m "First commit" file.txt &&
> + echo "commit b" >> file.txt &&
> + git commit -m "Second commit" file.txt &&
> + git switch -c foo &&
> + echo "commit c" >> file.txt &&
> + git commit -m "Third commit" file.txt &&
> + git branch --set-upstream-to=main &&
> + git switch main &&
> + git merge foo &&
> + git reset --hard HEAD^ &&
> + git switch foo &&
> + commit=$(git log -n1 --format="%h") &&
> + git rebase >out 2>err &&
> + test_must_be_empty out &&
> + cat <<-\OEF > expect &&
Why does this have to be orgiinal in such a strange way? When
everybody else uses string "EOF" as the end-of-here-doc-marker, and
if there is no downside to use the same string here, we should just
use the same "EOF" to avoid distracting readers.
> + warning: When "git rebase" is run without specifying <upstream> on the
> + command line, the current default is to use the fork-point
> + heuristics. This is expected to change in a future version
> + of Git, and you will have to explicitly give "--fork-point" from
> + the command line if you keep using the fork-point mode. You can
> + run "git config rebase.forkpoint false" to adopt the new default
> + in advance and that will also squelch the message.
> +
> + You can replace "git config" with "git config --global" to set a default
> + preference for all repositories. You can also pass --no-fork-point, --fork-point
> + on the command line to override the configured default per invocation.
> +
> + Successfully rebased and updated refs/heads/foo.
> + OEF
> + diff -qw expect err &&
Why not "test_cmp expect actual" like everybody else?
> +...
> + echo "Current branch foo is up to date." > expect &&
> + test_cmp out expect
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH v2] merge-tree: add -X strategy option
From: 唐宇奕 @ 2023-09-03 1:31 UTC (permalink / raw)
To: Izzy via GitGitGadget, Junio C Hamano; +Cc: git, newren
In-Reply-To: <CAFWsj_X=_PVyLcRgy77PL+kisjFqRSTCojB61B5cSK=6YROajg@mail.gmail.com>
Hello, is there any problem with this patch?
^ permalink raw reply
* Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Wesley @ 2023-09-03 2:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq4jkckuy7.fsf@gitster.g>
On 9/2/23 19:37, Junio C Hamano wrote:
> Wesley Schwengle <wesleys@opperschaap.net> writes:
Thanks for the feedback. I won't continue the patch series because some
of the feedback you've given below.
>> However doing so would trigger a different
>> kind of behavior. `git rebase <upstream>' behaves as if
>> `--no-fork-point' was supplied and without it behaves as if
>> `--fork-point' was supplied. This behavior can result in a loss of
>> commits and can surprise users.
>
> No, what is causing the loss in this particular case is allowing to
> use the fork-point heuristics. If you do not want it, you can
> either explicitly give --no-fork-point or <upstream> (or both if you
> feel that you need to absolutely be clear). Or you can set the
> configuration to "false" to disable this "auto" behaviour.
Isn't that what I'm saying? At least I'm trying to say what you are saying.
> By the way, while I do agree with the need to make users _aware_ of
> the "auto" behaviour [*1*], I am not yet convinced that there is a
> need to change the default in the future.
In that case, I'll abort this patch series. I don't agree with the `git
rebase' in the lazy form and `git rebase <upstream>' acting differently,
but I already have the rebase.forkpoint set to false to counter it.
> It might be better to extend the documentation instead, which will
> not distract those who are using the tool just fine already.
That is with the current viewpoints the best option I think.
>> + diff -qw expect err &&
>
> Why not "test_cmp expect actual" like everybody else?
As said in the initial patch series and the comment above the tests:
> There is one point where I'm a little confused, the `test_cmp' function in the
> testsuite doesn't like the output that is captured from STDERR, it seems that
> there is a difference in regards to whitespace. My workaround is to use
> `diff -wq`. I don't know if this is an accepted solution.
That's why.
Cheers,
Wesley
--
Wesley
Why not both?
^ permalink raw reply
* Re: [RFC] New configuration option "diff.statNameWidth"
From: Dragan Simic @ 2023-09-03 3:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq4jkcmdaw.fsf@gitster.g>
On 2023-09-03 00:16, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> Thank you very much for your thoughts and the time required to write
>> it all down. I'll do my best to make my patch(es) irresistible. :)
>
> Heh, I have to send the same from time to time, so it didn't take
> that much time.
Quite frankly, I suspected that to be a canned response, :) but anyway,
preparing that response, and possibly refining it over a couple of
iterations, surely already took a fair amount of time and effort.
> Having said all the above, once we start seeing the actual patches
> and its sales pitch (aka proposed commit log message), we do guide
> and help polishing the patch into a better shape, so it will not
> be like the contributor has to work in the dark without knowing
> what level of bar their contribution has to pass.
Thanks, everything sounds great and welcoming to the new contributors,
who of course need to be willing to put in the required amount of skill
and effort.
^ permalink raw reply
* Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Junio C Hamano @ 2023-09-03 4:50 UTC (permalink / raw)
To: Wesley Schwengle; +Cc: git
In-Reply-To: <xmqq4jkckuy7.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> If you rewind to lose commits from the branch you are (re)building
> against, and what was rewound and discarded was part of the work you
> are building, whether it is on a local branch or on a remote branch
> that contains what you have already pushed, they will be discarded,
> it is by design, and it is a known deficiency with the fork-point
> heuristics. How the fork-point heuristics breaks down is rather
> well known ...
Another tangent, this time very closely related to this topic, is
that it may be worth warning when the fork-point heuristics chooses
the base commit that is different from the original upstream,
regardless of how we ended up using fork-point heuristics.
Experienced users may not be confused when the heuristics kicks in
and when it does not (e.g. because they configured, because they
used the "lazy" form, or because they gave "--fork-point" from the
command line explicitly), but they still may get surprising results
if a reflog entry chosen to be used as the base by the heuristics is
not what they expected to be used, and can lose their work that way.
Imagine that you pushed your work to the remote that is a shared
repository, and then continued building on top of it, while others
rewound the remote branch to eject your work, and your "git fetch"
updated the remote-tracking branch. You'll be pretty much in the
same situation you had in your reproduction recipe that rewound your
own local branch that you used to build your derived work on and
would lose your work the same way, if you do not notice that the
remote branch has been rewound (and the fork-point heuristics chose
a "wrong" commit from the reflog of your remote-tracking branch.
Perhaps something along the lines of this (not even compile tested,
though)... It might even be useful to show a shortlog between the
.restrict_revision and .upstream, which is the list of commits that
is potentially lost, but that might turn out to be excessively loud
and noisy in the workflow of those who do benefit from the
fork-point heuristics because their project rewinds branches too
often and too wildly for them to manually keep track of. I dunno.
builtin/rebase.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git c/builtin/rebase.c w/builtin/rebase.c
index 50cb85751f..432a97e205 100644
--- c/builtin/rebase.c
+++ w/builtin/rebase.c
@@ -1721,9 +1721,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (keep_base && options.reapply_cherry_picks)
options.upstream = options.onto;
- if (options.fork_point > 0)
+ if (options.fork_point > 0) {
options.restrict_revision =
get_fork_point(options.upstream_name, options.orig_head);
+ if (options.restrict_revision &&
+ options.restrict_revision != options.upstream)
+ warning(_("fork-point heuristics using %s from the reflog of %s"),
+ oid_to_hex(&options.restrict_revision->object.oid),
+ options.upstream_name);
+ }
if (repo_read_index(the_repository) < 0)
die(_("could not read index"));
^ permalink raw reply related
* Re: Plumbing for mapping from a remote tracking ref to the remote ref?
From: Tao Klerks @ 2023-09-03 7:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <xmqqczf5lgk3.fsf@gitster.g>
On Sun, Jun 19, 2022 at 1:04 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> $ git refmap refs/remotes/somepath/{branch-A,branch-B}
> >> origin refs/heads/branch-A
> >> origin refs/heads/branch-B
> >>
> >> IOW, you give name(s) of remote-tracking branches and then you get
> >> the remote and their ref for these?
> >
> > Modulo introducing a new top-level command (a subcommand of `git remote`
> > would make much more sense and make the feature eminently more
> > discoverable), and modulo allowing patterns in the ref to match, I agree.
>
> "git remote" is primarily about "I have this remote---tell me more
> about it", but this query goes in the other direction, and that is
> why I threw a non-existing command to solicit alternatives that are
> potentially better than "git remote".
Thank you for the responses here, and my apologies for not following
up (much) earlier.
Given that "git remote" already deals with different types of args
(remotes, URLs, remote branches), could it make sense to introduce a
dedicated new subcommand, not directly related to "set-branches", eg
"map-refs"? I agree with Dscho that keeping it under "git remote"
would help with discoverability and avoid clutter in the global
namespace: Git already has many top-level commands, the "theme" under
which this one fits is definitely "remote stuff", and "git remote"
already does a number of substantially-different things all related
with *remote configuration*.
In fact that's another way of seeing things: most of "git remote"'s
current subcommands are just syntactic sugar over "git config" (the
two that operate outside of the config are "set-head" and "update"),
and this new one would be config-focused in exactly the same way.
To Junio's question along the lines of "what if someone mapped
multiple remote namespaces to a single 'tracking namespace' location
in the local repo?" (which I hope is rare - I seem to recall there are
at least some operations that warn when this is detected), this
ambiguity would be absent in a "git remote" subcommand, as it would
take a remote name.
I had also considered some new weird "@{remotemapping}"-style syntax
to rev-parse, but here precisely there *would* be no implicit remote
context, and so getting more than one answer would be an option, and
that doesn't make sense for rev-parse.
Regarding patterns and wildcards, for *my* purpose at least, they
don't make much sense: The whole purpose of the exercise is to say "I
know the ref I want updated in my repo, I know what remote that it is
mapped to or that I want to update it from, I want to know exactly
what to put in a "git fetch <remote_name> <remote_ref>..." call, to
get that ref updated correctly/consistently for the current repo,
without affecting any other refs that this repo has mapped for that
remote.
Would something like the following be mutually agreeable?
$ git remote origin map-ref
refs/remotes/my-favorite-remotes/origin/someref
refs/heads/someref
$ git remote origin map-ref
refs/remotes/my-favorite-remotes/origin/someref
refs/remotes/my-favorite-remotes/origin/someotherref
refs/heads/someref
refs/heads/someotherref
$ git remote origin map-ref refs/remotes/someotherpath/someref
error: the ref "refs/remotes/someotherpath/someref" is not
mapped in any configured refspec of remote "origin".
Thanks,
Tao
^ permalink raw reply
* Re: [PATCH 1/2] ci: allow branch selection through "vars"
From: Johannes Schindelin @ 2023-09-03 8:59 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20230830195113.GA1709824@coredump.intra.peff.net>
Hi Jeff,
On Wed, 30 Aug 2023, Jeff King wrote:
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 1b41278a7f..c364abb8f8 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -21,6 +21,7 @@ concurrency:
> jobs:
> ci-config:
> name: config
> + if: vars.CI_BRANCHES == '' || contains(vars.CI_BRANCHES, github.ref_name)
This might be too loose a check, as branch names that are a substring of
any name listed in `CI_BRANCHES` would be false positive match. For
example, if `CI_BRANCHES` was set to `maint next seen`, a branch called
`see` would be a false match.
Due to the absence of a `concat()` function (for more details, see
https://docs.github.com/en/actions/learn-github-actions/expressions#functions),
I fear that we'll have to resort to something like `contains(format(' {0} ',
vars.CI_BRANCHES), format(' {0} ', github.ref_name))`.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v2 1/3] range-diff: treat notes like `log`
From: Johannes Schindelin @ 2023-09-03 12:17 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, Denton Liu, Jeff King
In-Reply-To: <e9a59108311369d8197b9870a8810d5283ec124f.1693584310.git.code@khaugsbakk.name>
[-- Attachment #1: Type: text/plain, Size: 4720 bytes --]
Hi Kristoffer,
On Fri, 1 Sep 2023, Kristoffer Haugsbakk wrote:
> Currently, `range-diff` shows the default notes if no notes-related
> arguments are given. This is also how `log` behaves. But unlike
> `range-diff`, `log` does *not* show the default notes if
> `--notes=<custom>` are given. In other words, this:
>
> git log --notes=custom
>
> is equivalent to this:
>
> git log --no-notes --notes=custom
>
> While:
>
> git range-diff --notes=custom
>
> acts like this:
>
> git log --notes --notes-custom
>
> This can’t be how the user expects `range-diff` to behave given that the
> man page for `range diff` under `--[no-]notes[=<ref>]` says:
>
> > This flag is passed to the git log program (see git-log(1)) that
> > generates the patches.
>
> This behavior also affects `format-patch` since it uses `range-diff` for
> the cover letter. Unlike `log`, though, `format-patch` is not supposed
> to show the default notes if no notes-related arguments are given.[1]
> But this promise is broken when the range-diff happens to have something
> to say about the changes to the default notes, since that will be shown
> in the cover letter.
Very well explained.
The root cause for this is 8cf51561d1e (range-diff: fix a crash in parsing
git-log output, 2020-04-15) which added the `--notes` argument in
`read_patches()`' call. The commit message explains why this is needed:
the (necessary) `--pretty=medium` would turn off the notes, therefore
`--notes` had to be added to reinstate the original behavior (except, as
you pointed out, in the case `--notes=<ref>` was specified).
> Remedy this by co-opting the `--standard-notes` option which has been
> deprecated since ab18b2c0df[2] and which is currently only documented in
> `pretty-options`.
This sounds a bit less desirable, though, than passing the `--notes`
argument only as needed. This patch (on top of `notes-range-diff` in your
fork) lets the new test still pass while leaving `--standard-notes`
deprecated:
-- snipsnap --
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 8c982609c99..dc685be363a 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -88,6 +88,7 @@ being displayed. Examples: "--notes=foo" will show only notes from
from "refs/notes/bar".
--show-notes[=<ref>]::
+--[no-]standard-notes::
These options are deprecated. Use the above --notes/--no-notes
options instead.
endif::git-rev-list[]
diff --git a/range-diff.c b/range-diff.c
index f070e4a4ceb..fbb81a92cc0 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -41,12 +41,20 @@ static int read_patches(const char *range, struct string_list *list,
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;
struct patch_util *util = NULL;
- int in_header = 1;
+ int i, implicit_notes_arg = 1, in_header = 1;
char *line, *current_filename = NULL;
ssize_t len;
size_t size;
int ret = -1;
+ for (i = 0; other_arg && i < other_arg->nr; i++)
+ if (!strcmp(other_arg->v[i], "--notes") ||
+ starts_with(other_arg->v[i], "--notes=") ||
+ !strcmp(other_arg->v[i], "--no-notes")) {
+ implicit_notes_arg = 0;
+ break;
+ }
+
strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
"--reverse", "--date-order", "--decorate=no",
"--no-prefix", "--submodule=short",
@@ -60,8 +68,9 @@ static int read_patches(const char *range, struct string_list *list,
"--output-indicator-context=#",
"--no-abbrev-commit",
"--pretty=medium",
- "--standard-notes",
NULL);
+ if (implicit_notes_arg)
+ strvec_push(&cp.args, "--notes");
strvec_push(&cp.args, range);
if (other_arg)
strvec_pushv(&cp.args, other_arg->v);
diff --git a/revision.c b/revision.c
index 44a04004a70..2f4c53ea207 100644
--- a/revision.c
+++ b/revision.c
@@ -2495,13 +2495,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
disable_display_notes(&revs->notes_opt, &revs->show_notes);
revs->show_notes_given = 1;
} else if (!strcmp(arg, "--standard-notes")) {
- disable_display_notes(&revs->notes_opt, &revs->show_notes);
- revs->show_notes_given = 0;
- enable_default_display_notes(&revs->notes_opt,
- &revs->show_notes);
- revs->notes_opt.use_default_notes = -1;
+ revs->show_notes_given = 1;
+ revs->notes_opt.use_default_notes = 1;
} else if (!strcmp(arg, "--no-standard-notes")) {
- /* Deprecated */
revs->notes_opt.use_default_notes = 0;
} else if (!strcmp(arg, "--oneline")) {
revs->verbose_header = 1;
^ permalink raw reply related
* Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Wesley Schwengle @ 2023-09-03 12:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqlednuagl.fsf@gitster.g>
On 9/3/23 00:50, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> If you rewind to lose commits from the branch you are (re)building
>> against, and what was rewound and discarded was part of the work you
>> are building, whether it is on a local branch or on a remote branch
>> that contains what you have already pushed, they will be discarded,
>> it is by design, and it is a known deficiency with the fork-point
>> heuristics. How the fork-point heuristics breaks down is rather
>> well known ...
>
> Another tangent, this time very closely related to this topic, is
> that it may be worth warning when the fork-point heuristics chooses
> the base commit that is different from the original upstream,
> regardless of how we ended up using fork-point heuristics.
>
> [snip]
>
> Perhaps something along the lines of this (not even compile tested,
> though)... It might even be useful to show a shortlog between the
> .restrict_revision and .upstream, which is the list of commits that
> is potentially lost, but that might turn out to be excessively loud
> and noisy in the workflow of those who do benefit from the
> fork-point heuristics because their project rewinds branches too
> often and too wildly for them to manually keep track of. I dunno.
I like the idea of the warning, but it could be loud indeed and you'll
want to turn it off in that case.
--
Wesley
^ permalink raw reply
* [PATCH] sequencer: update abort safety file more sparingly
From: Oswald Buddenhagen @ 2023-09-03 15:11 UTC (permalink / raw)
To: git; +Cc: Stephan Beyer, Johannes Schindelin, Phillip Wood
The only situation where the file's content matters is --continue'ing
(after a multi-cherry-pick merge conflict). This means that it is
sufficient to write it in a single place, when we are prematurely
exiting the main workhorse. This is much easier to reason about than the
three dispersed calls originally introduced in 1e41229d ("sequencer:
make sequencer abort safer", 2016-12-07). We now can also remove the
inefficient file-based check whether the file needs writing, which
wasn't even reliable: a single pick executed during an interrupted
sequence would bypass the safety.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
Cc: Stephan Beyer <s-beyer@gmx.net>
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
sequencer.c | 9 ++-------
t/t3510-cherry-pick-sequence.sh | 9 +++++++++
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index a66dcf8ab2..716384cc7b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -575,10 +575,6 @@ static void update_abort_safety_file(void)
{
struct object_id head;
- /* Do nothing on a single-pick */
- if (!file_exists(git_path_seq_dir()))
- return;
-
if (!repo_get_oid(the_repository, "HEAD", &head))
write_file(git_path_abort_safety_file(), "%s", oid_to_hex(&head));
else
@@ -618,7 +614,6 @@ static int fast_forward_to(struct repository *r,
strbuf_release(&sb);
strbuf_release(&err);
ref_transaction_free(transaction);
- update_abort_safety_file();
return 0;
}
@@ -2435,7 +2430,6 @@ static int do_pick_commit(struct repository *r,
free_message(commit, &msg);
free(author);
strbuf_release(&msgbuf);
- update_abort_safety_file();
return res;
}
@@ -5269,8 +5263,9 @@ int sequencer_pick_revisions(struct repository *r,
return -1;
if (save_opts(opts))
return -1;
- update_abort_safety_file();
res = pick_commits(r, &todo_list, opts);
+ if (todo_list.current < todo_list.nr)
+ update_abort_safety_file();
todo_list_release(&todo_list);
return res;
}
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 3b0fa66c33..170b664c33 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -318,6 +318,15 @@ test_expect_success '--abort does not unsafely change HEAD' '
test_cmp_rev base HEAD
'
+test_expect_success '--abort after single pick does not unsafely change HEAD' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick picked anotherpick &&
+ git reset --hard &&
+ git cherry-pick unrelatedpick &&
+ git cherry-pick --abort 2>actual &&
+ test_i18ngrep "You seem to have moved HEAD" actual
+'
+
test_expect_success 'cherry-pick --abort to cancel multiple revert' '
pristine_detach anotherpick &&
test_expect_code 1 git revert base..picked &&
--
2.40.0.152.g15d061e6df
^ permalink raw reply related
* [PATCH] sequencer: remove unreachable exit condition in pick_commits()
From: Oswald Buddenhagen @ 2023-09-03 15:11 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Phillip Wood
This was introduced by 56dc3ab04 ("sequencer (rebase -i): implement the
'edit' command", 2017-01-02), and was pointless from the get-go, as the
command causes an early return.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
this remains valid after phillip's pending series, through it becomes
marginally harder to prove (c.f. "sequencer: factor out part of
pick_commits()").
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
sequencer.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index a66dcf8ab2..99e9c520ca 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4832,10 +4832,6 @@ static int pick_commits(struct repository *r,
struct strbuf head_ref = STRBUF_INIT, buf = STRBUF_INIT;
struct stat st;
- /* Stopped in the middle, as planned? */
- if (todo_list->current < todo_list->nr)
- return 0;
-
if (read_oneliner(&head_ref, rebase_path_head_name(), 0) &&
starts_with(head_ref.buf, "refs/")) {
const char *msg;
--
2.40.0.152.g15d061e6df
^ permalink raw reply related
* [PATCH] sequencer: fix error message on failure to copy SQUASH_MSG
From: Oswald Buddenhagen @ 2023-09-03 15:11 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano
The message talked about renaming, while the actual action is copying.
This was introduced by 6e98de72c ("sequencer (rebase -i): add support
for the 'fixup' and 'squash' commands", 2017-01-02).
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
i didn't try verifying whether the action shouldn't be actually be a
move, as i'm getting lost in this forest of files.
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: Phillip Wood <phillip.wood123@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>
---
totally on a tangent, does someone feel like teaching copy_file() to try
ioctl(FICLONE) (i.e., reflink) first?
---
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index a66dcf8ab2..2f3d7d4eee 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2320,7 +2320,7 @@ static int do_pick_commit(struct repository *r,
const char *dest = git_path_squash_msg(r);
unlink(dest);
if (copy_file(dest, rebase_path_squash_msg(), 0666)) {
- res = error(_("could not rename '%s' to '%s'"),
+ res = error(_("could not copy '%s' to '%s'"),
rebase_path_squash_msg(), dest);
goto leave;
}
--
2.40.0.152.g15d061e6df
^ permalink raw reply related
* Re: [PATCH 11/14] replay: use standard revision ranges
From: Johannes Schindelin @ 2023-09-03 15:47 UTC (permalink / raw)
To: Elijah Newren
Cc: Derrick Stolee, Christian Couder, git, Junio C Hamano,
Patrick Steinhardt, John Cai, Christian Couder
In-Reply-To: <CABPp-BGRtcBQ_6fkMrTskV9dk71ffycXZ8hEE_RaOrAdza_wLA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2812 bytes --]
Hi Elijah & Stolee,
On Sat, 29 Apr 2023, Elijah Newren wrote:
> On Mon, Apr 24, 2023 at 8:23 AM Derrick Stolee <derrickstolee@github.com> wrote:
> >
> > On 4/22/2023 9:18 PM, Elijah Newren wrote:
> > > On Thu, Apr 20, 2023 at 6:44 AM Derrick Stolee <derrickstolee@github.com> wrote:
> > >>
> > >> On 4/20/2023 12:53 AM, Elijah Newren wrote:
> > >>> On Tue, Apr 18, 2023 at 6:10 AM Derrick Stolee <derrickstolee@github.com> wrote:
> >
> > >> 3. (Ordering options) Modifications to how those commits are ordered,
> > >> such as --topo-order, --date-order, and --reverse. These seem to
> > >> be overridden by git-replay (although, --reverse probably causes
> > >> some confusion right now).
> > >
> > > Yep, intentionally overridden.
> > >
> > > Could you elaborate on what you mean by --reverse causing confusion?
> >
> > It's very unlikely that a list of patches will successfully apply
> > when applied in the reverse order. If we allow the argument, then
> > someone will try it thinking they can flip their commits. Then they
> > might be surprised when there are a bunch of conflicts on every
> > commit.
> >
> > Basically, I'm not super thrilled about exposing options that are
> > unlikely to be valuable to users and instead are more likely to cause
> > confusion due to changes that won't successfully apply.
>
> Oh, I got thrown by the "right now" portion of your comment; I
> couldn't see how time or future changes would affect anything to make
> it less (or more) confusing for users.
>
> Quick clarification, though: while you correctly point out the type of
> confusion the user would experience without my overriding, my
> overriding of rev.reverse (after setup_revisions() returns, not before
> it is called) precludes that experience. The override means none of
> the above happens, and they would instead just wonder why their option
> is being ignored.
FWIW here is my view on the matter: `git replay`, at least in its current
incarnation, is a really low-level tool. As such, I actually do not want
to worry much about protecting users from nonsensical invocations.
In that light, I would like to see that code rejecting all revision
options except `--diff-algorithm` be dropped. Should we ever decide to add
a non-low-level mode to `git replay`, we can easily add some user-friendly
sanity check of the options then, and only for that non-low-level code.
For now, I feel that it's just complicating things, and `git replay` is in
the experimental phase anyway.
And further, I would even like to see that `--reverse` override go, and
turn it into `revs.reverse = !revs.reverse` instead. (And yes, I can
easily think of instances where I would have wanted to reverse a series of
patches...).
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] sequencer: update abort safety file more sparingly
From: Phillip Wood @ 2023-09-03 18:40 UTC (permalink / raw)
To: Oswald Buddenhagen, git; +Cc: Stephan Beyer, Johannes Schindelin
In-Reply-To: <20230903151132.739166-1-oswald.buddenhagen@gmx.de>
Hi Oswald
On 03/09/2023 16:11, Oswald Buddenhagen wrote:
> The only situation where the file's content matters is --continue'ing
> (after a multi-cherry-pick merge conflict).
I don't think "cherry-pick --continue" consults the abort safety file,
it only matters for "cherry-pick --skip" and "cherry-pick --abort".
> This means that it is
> sufficient to write it in a single place, when we are prematurely
> exiting the main workhorse.
I think this introduces a regression because the safety file will not
get updated when "cherry-pick --continue" stops for the user to resolve
conflicts.
> This is much easier to reason about than the
> three dispersed calls originally introduced in 1e41229d ("sequencer:
> make sequencer abort safer", 2016-12-07). We now can also remove the
> inefficient file-based check whether the file needs writing, which
> wasn't even reliable: a single pick executed during an interrupted
> sequence would bypass the safety.
An alternate view is that the abort safety file exists to prevent the
user losing commits that have not been cherry-picked and it is desirable
to be able to abort after cherry-picking a single pick in the middle of
a sequence of cherry-picks.
Best Wishes
Phillip
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> ---
> Cc: Stephan Beyer <s-beyer@gmx.net>
> Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
> Cc: Phillip Wood <phillip.wood123@gmail.com>
> ---
> sequencer.c | 9 ++-------
> t/t3510-cherry-pick-sequence.sh | 9 +++++++++
> 2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index a66dcf8ab2..716384cc7b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -575,10 +575,6 @@ static void update_abort_safety_file(void)
> {
> struct object_id head;
>
> - /* Do nothing on a single-pick */
> - if (!file_exists(git_path_seq_dir()))
> - return;
> -
> if (!repo_get_oid(the_repository, "HEAD", &head))
> write_file(git_path_abort_safety_file(), "%s", oid_to_hex(&head));
> else
> @@ -618,7 +614,6 @@ static int fast_forward_to(struct repository *r,
> strbuf_release(&sb);
> strbuf_release(&err);
> ref_transaction_free(transaction);
> - update_abort_safety_file();
> return 0;
> }
>
> @@ -2435,7 +2430,6 @@ static int do_pick_commit(struct repository *r,
> free_message(commit, &msg);
> free(author);
> strbuf_release(&msgbuf);
> - update_abort_safety_file();
>
> return res;
> }
> @@ -5269,8 +5263,9 @@ int sequencer_pick_revisions(struct repository *r,
> return -1;
> if (save_opts(opts))
> return -1;
> - update_abort_safety_file();
> res = pick_commits(r, &todo_list, opts);
> + if (todo_list.current < todo_list.nr)
> + update_abort_safety_file();
> todo_list_release(&todo_list);
> return res;
> }
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 3b0fa66c33..170b664c33 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -318,6 +318,15 @@ test_expect_success '--abort does not unsafely change HEAD' '
> test_cmp_rev base HEAD
> '
>
> +test_expect_success '--abort after single pick does not unsafely change HEAD' '
> + pristine_detach initial &&
> + test_must_fail git cherry-pick picked anotherpick &&
> + git reset --hard &&
> + git cherry-pick unrelatedpick &&
> + git cherry-pick --abort 2>actual &&
> + test_i18ngrep "You seem to have moved HEAD" actual
> +'
> +
> test_expect_success 'cherry-pick --abort to cancel multiple revert' '
> pristine_detach anotherpick &&
> test_expect_code 1 git revert base..picked &&
^ permalink raw reply
* Re: [PATCH] sequencer: update abort safety file more sparingly
From: Oswald Buddenhagen @ 2023-09-03 19:25 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Stephan Beyer, Johannes Schindelin
In-Reply-To: <29fb7a38-1e92-457a-93ff-0e64ac09b907@gmail.com>
On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote:
>On 03/09/2023 16:11, Oswald Buddenhagen wrote:
>> The only situation where the file's content matters is --continue'ing
>> (after a multi-cherry-pick merge conflict).
>
>I don't think "cherry-pick --continue" consults the abort safety file,
>
duh, obvious blunder.
>it only matters for "cherry-pick --skip"
>
that doesn't seem right. a --skip is just a --continue with a prior
reset, more or less.
>and "cherry-pick --abort".
>
that one, of course.
>> This means that it is
>> sufficient to write it in a single place, when we are prematurely
>> exiting the main workhorse.
>
>I think this introduces a regression because the safety file will not
>get updated when "cherry-pick --continue" stops for the user to resolve
>conflicts.
>
true, there is indeed this second entry point.
i'll try to find a better "choke point".
>> which wasn't even reliable: a single pick executed during an
>> interrupted sequence would bypass the safety.
>
>An alternate view is that the abort safety file exists to prevent the
>user losing commits that have not been cherry-picked and it is
>desirable to be able to abort after cherry-picking a single pick in the
>middle of a sequence of cherry-picks.
>
if you did a fresh commit before or after the single pick, you'd lose
it.
also, the feature doesn't actually prevent aborting, only the automatic
reset.
regards
^ permalink raw reply
* Re: [PATCH] sequencer: update abort safety file more sparingly
From: Phillip Wood @ 2023-09-03 19:48 UTC (permalink / raw)
To: Oswald Buddenhagen, phillip.wood; +Cc: git, Stephan Beyer, Johannes Schindelin
In-Reply-To: <ZPTdmnHfDcTBqaSl@ugly>
On 03/09/2023 20:25, Oswald Buddenhagen wrote:
> On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote:
>> On 03/09/2023 16:11, Oswald Buddenhagen wrote:
>>> The only situation where the file's content matters is --continue'ing
>>> (after a multi-cherry-pick merge conflict).
>>
>> I don't think "cherry-pick --continue" consults the abort safety file,
> duh, obvious blunder.
>
>> it only matters for "cherry-pick --skip"
>>
> that doesn't seem right. a --skip is just a --continue with a prior
> reset, more or less.
sequencer_skip() calls rollback_is_safe() which checks the abort safety
file.
>> and "cherry-pick --abort".
>>
> that one, of course.
>
>>> This means that it is
>>> sufficient to write it in a single place, when we are prematurely
>>> exiting the main workhorse.
>>
>> I think this introduces a regression because the safety file will not
>> get updated when "cherry-pick --continue" stops for the user to
>> resolve conflicts.
>>
> true, there is indeed this second entry point.
> i'll try to find a better "choke point".
I think that is probably tricky, I'm not really clear what the
aim/purpose of this refactoring is.
>>> which wasn't even reliable: a single pick executed during an
>>> interrupted sequence would bypass the safety.
>>
>> An alternate view is that the abort safety file exists to prevent the
>> user losing commits that have not been cherry-picked and it is
>> desirable to be able to abort after cherry-picking a single pick in
>> the middle of a sequence of cherry-picks.
>>
> if you did a fresh commit before or after the single pick, you'd lose it.
> also,
Oh, I can see that you'd lose a commit made before a single pick but I
don't see how you'd lose a commit made after it. I'm still not convinced
it is a particularly helpful change.
> the feature doesn't actually prevent aborting, only the automatic
> reset.
Oh right, it removes the state directory but leaves HEAD untouched if it
does not match the commit recorded in the abort safety file.
Best Wishes
Phillip
> regards
^ permalink raw reply
* Re: [PATCH] sequencer: update abort safety file more sparingly
From: Oswald Buddenhagen @ 2023-09-03 20:18 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Stephan Beyer, Johannes Schindelin
In-Reply-To: <fdf80c36-0e28-44f3-9cef-85d38d2d48f1@gmail.com>
On Sun, Sep 03, 2023 at 08:48:14PM +0100, Phillip Wood wrote:
>On 03/09/2023 20:25, Oswald Buddenhagen wrote:
>> On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote:
>>> it only matters for "cherry-pick --skip"
>>>
>> that doesn't seem right. a --skip is just a --continue with a prior
>> reset, more or less.
>
>sequencer_skip() calls rollback_is_safe() which checks the abort safety
>file.
>
that's weird. can you think of a good reason for doing that?
>> i'll try to find a better "choke point".
>
>I think that is probably tricky,
>
yeah
>I'm not really clear what the aim/purpose of this refactoring is.
>
to make my head not explode.
more specifically, to get it out of the way of the rebase path, which is
what i'm actually concerned with.
generally, i think this whole ad-hoc state management is a nightmare,
and i'd be surprised if there weren't some more loose ends.
i think i'd aim for an object-oriented-ish design with an encapsulated
state, lazy loading getters, lazy setters, and a commit entry point (or
maybe several partial ones). no idea how that would play out.
>> if you did a fresh commit before or after the single pick, you'd lose
>> it.
>
>Oh, I can see that you'd lose a commit made before a single pick but I
>don't see how you'd lose a commit made after it.
>
right. thinko. it's a bit late here. ^^
regards
^ permalink raw reply
* Re: [PATCH 1/2] am: pay attention to user-defined context size
From: tony raynes @ 2023-09-03 21:29 UTC (permalink / raw)
To: gitgitgadget@gmail.com
Cc: Johannes.Schindelin@gmx.de, git@vger.kernel.org,
gitster@pobox.com, newren@gmail.com, plroskin@gmail.com
^ permalink raw reply
* [PATCH] var: avoid a segmentation fault when `HOME` is unset
From: Johannes Schindelin via GitGitGadget @ 2023-09-04 6:21 UTC (permalink / raw)
To: git; +Cc: brian m. carlson, Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The code introduced in 576a37fccbf (var: add attributes files locations,
2023-06-27) paid careful attention to use `xstrdup()` for pointers known
never to be `NULL`, and `xstrdup_or_null()` otherwise.
One spot was missed, though: `git_attr_global_file()` can return `NULL`,
when the `HOME` variable is not set (and neither `XDG_CONFIG_HOME`), a
scenario not too uncommon in certain server scenarios.
Fix this, and add a test case to avoid future regressions.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
var: avoid a segmentation fault when HOME is unset
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1580%2Fdscho%2Favoid-segfault-in-git-var-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1580/dscho/avoid-segfault-in-git-var-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1580
builtin/var.c | 2 +-
t/t0007-git-var.sh | 9 +++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/builtin/var.c b/builtin/var.c
index 74161bdf1c6..8cf7dd9e2e5 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -66,7 +66,7 @@ static char *git_attr_val_system(int ident_flag UNUSED)
static char *git_attr_val_global(int ident_flag UNUSED)
{
- char *file = xstrdup(git_attr_global_file());
+ char *file = xstrdup_or_null(git_attr_global_file());
if (file) {
normalize_path_copy(file, file);
return file;
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index 8cb597f99c4..ff4fd9348cc 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -268,4 +268,13 @@ test_expect_success 'listing and asking for variables are exclusive' '
test_must_fail git var -l GIT_COMMITTER_IDENT
'
+test_expect_success '`git var -l` works even without HOME' '
+ (
+ XDG_CONFIG_HOME= &&
+ export XDG_CONFIG_HOME &&
+ unset HOME &&
+ git var -l
+ )
+'
+
test_done
base-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40
--
gitgitgadget
^ permalink raw reply related
* Azure Repos
From: M Hickford @ 2023-09-04 7:00 UTC (permalink / raw)
To: Git Mailing List
Hi. Anyone use Azure Repos (dev.azure.com)
https://azure.microsoft.com/en-us/products/devops/repos ? I'm looking
for help to test credential helper git-credential-azure
https://github.com/hickford/git-credential-azure
^ permalink raw reply
* Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable
From: Phillip Wood @ 2023-09-04 9:56 UTC (permalink / raw)
To: Jeff King, phillip.wood; +Cc: git, Johannes Schindelin
In-Reply-To: <20230901173214.GA1947546@coredump.intra.peff.net>
On 01/09/2023 18:32, Jeff King wrote:
> On Fri, Sep 01, 2023 at 02:24:59PM +0100, Phillip Wood wrote:
>
>> On 30/08/2023 20:49, Jeff King wrote:
>>> This is a more efficient way to do the same thing that
>>> ci/config/allow-ref does (which didn't exist back then).
>>
>> I like the idea of a more efficient way to skip the ci for certain refs.
>> I've got my allow-ref script set up to reject a bunch of refs and run the ci
>> on everything else. It's not clear to me how to replicate that with the
>> setup proposed here. Would it be possible to add a second variable that
>> prevents the ci from being run if it contains ref being pushed?
>
> Drat, I was hoping nobody was using it that way. :)
Sorry to be a pain.
> Yes, I think it would be possible to do something like:
>
> if: |
> (vars.CI_BRANCHES == '' || contains(vars.CI_BRANCHES, github.ref_name)) &&
> !contains(vars.CI_BRANCHES_REJECT, github.ref_name)
>
> It doesn't allow globbing, though. Do you need that?
Oh I'd missed that, yes I do. All the globs are prefix matches but I'm
not sure that helps.
Best Wishes
Phillip
> -Peff
^ permalink raw reply
* Re: [PATCH] sequencer: update abort safety file more sparingly
From: Phillip Wood @ 2023-09-04 10:05 UTC (permalink / raw)
To: Oswald Buddenhagen, phillip.wood; +Cc: git, Stephan Beyer, Johannes Schindelin
In-Reply-To: <ZPTqEIvW3zJ4eafT@ugly>
On 03/09/2023 21:18, Oswald Buddenhagen wrote:
> On Sun, Sep 03, 2023 at 08:48:14PM +0100, Phillip Wood wrote:
>> On 03/09/2023 20:25, Oswald Buddenhagen wrote:
>>> On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote:
>>>> it only matters for "cherry-pick --skip"
>>>>
>>> that doesn't seem right. a --skip is just a --continue with a prior
>>> reset, more or less.
>>
>> sequencer_skip() calls rollback_is_safe() which checks the abort
>> safety file.
>>
> that's weird. can you think of a good reason for doing that?
I think it is clear from the code - so it does not reset changes after
the user has committed the conflict resolution.
>>> i'll try to find a better "choke point".
>>
>> I think that is probably tricky,
>>
> yeah
>
>> I'm not really clear what the aim/purpose of this refactoring is.
>>
> to make my head not explode.
> more specifically, to get it out of the way of the rebase path, which is
> what i'm actually concerned with.
rebase and cherry-pick share the same code path most of the time. In
particular "cherry-pick --continue" and "rebase --continue" both use
sequencer_continue() as their entry point so I think the best you can do
is guard the calls to update_abort_safety_file() with "if
(!is_rebase_i(opts))" or add "if (is_rebase_i(opts)) return" to the
start of update_abort_safety_file().
> generally, i think this whole ad-hoc state management is a nightmare,
> and i'd be surprised if there weren't some more loose ends.
> i think i'd aim for an object-oriented-ish design with an encapsulated
> state, lazy loading getters, lazy setters, and a commit entry point (or
> maybe several partial ones). no idea how that would play out.
I've been working on something similar to only write the state to disc
when the sequencer stops for user interaction. I'm hoping to have the
first set of patches ready to submit in the next development cycle. You
can see the branch at [1]. It is very much a work in progress at the
moment, the code is mostly OK (I'm running it in my git build) but some
commits are empty, others need splitting and the commit messages need a
lot of work. The basic idea is to add a private struct that holds the
state and write that to disc when pick_commits() returns.
Best Wishes
Phillip
[1] https://github.com/phillipwood/git/commits/wip/sequencer-context
>>> if you did a fresh commit before or after the single pick, you'd lose
>>> it.
>>
>> Oh, I can see that you'd lose a commit made before a single pick but I
>> don't see how you'd lose a commit made after it.
>>
> right. thinko. it's a bit late here. ^^
>
> regards
^ permalink raw reply
* Re: [PATCH] sequencer: fix error message on failure to copy SQUASH_MSG
From: Phillip Wood @ 2023-09-04 10:10 UTC (permalink / raw)
To: Oswald Buddenhagen, git; +Cc: Johannes Schindelin, Junio C Hamano
In-Reply-To: <20230903151132.739136-1-oswald.buddenhagen@gmx.de>
On 03/09/2023 16:11, Oswald Buddenhagen wrote:
> The message talked about renaming, while the actual action is copying.
> This was introduced by 6e98de72c ("sequencer (rebase -i): add support
> for the 'fixup' and 'squash' commands", 2017-01-02).
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> ---
> i didn't try verifying whether the action shouldn't be actually be a
> move, as i'm getting lost in this forest of files.
This looks good, thanks. I think copying the file is OK. We're creating
.git/SQUASH_MSG as a convenience in case the user decides to commit the
conflict resolution themselves rather than leaving it to "rebase
--continue". I don't think we reuse the contents of
rebase_path_squash_msg() when tho rebase continues but copying it
certainly isn't wrong and we're not in a hot path where we need to worry
about the cost of copying a small file.
Best Wishes
Phillip
> Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
> Cc: Phillip Wood <phillip.wood123@gmail.com>
> Cc: Junio C Hamano <gitster@pobox.com>
>
> ---
> totally on a tangent, does someone feel like teaching copy_file() to try
> ioctl(FICLONE) (i.e., reflink) first?
> ---
> sequencer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index a66dcf8ab2..2f3d7d4eee 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2320,7 +2320,7 @@ static int do_pick_commit(struct repository *r,
> const char *dest = git_path_squash_msg(r);
> unlink(dest);
> if (copy_file(dest, rebase_path_squash_msg(), 0666)) {
> - res = error(_("could not rename '%s' to '%s'"),
> + res = error(_("could not copy '%s' to '%s'"),
> rebase_path_squash_msg(), dest);
> goto leave;
> }
^ permalink raw reply
* Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Phillip Wood @ 2023-09-04 10:16 UTC (permalink / raw)
To: Junio C Hamano, Wesley Schwengle; +Cc: git
In-Reply-To: <xmqqlednuagl.fsf@gitster.g>
On 03/09/2023 05:50, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> If you rewind to lose commits from the branch you are (re)building
>> against, and what was rewound and discarded was part of the work you
>> are building, whether it is on a local branch or on a remote branch
>> that contains what you have already pushed, they will be discarded,
>> it is by design, and it is a known deficiency with the fork-point
>> heuristics. How the fork-point heuristics breaks down is rather
>> well known ...
>
> Another tangent, this time very closely related to this topic, is
> that it may be worth warning when the fork-point heuristics chooses
> the base commit that is different from the original upstream,
> regardless of how we ended up using fork-point heuristics.
I think that is a good idea and would help to mitigate the surprise that
some users have expressed when --fork-point kicks and they didn't know
about it. I think we may want to compare "branch_base" which holds the
merge-base of HEAD and upstream with "restrict_revision" to decide when
to warn.
Best Wishes
Phillip
> Experienced users may not be confused when the heuristics kicks in
> and when it does not (e.g. because they configured, because they
> used the "lazy" form, or because they gave "--fork-point" from the
> command line explicitly), but they still may get surprising results
> if a reflog entry chosen to be used as the base by the heuristics is
> not what they expected to be used, and can lose their work that way.
> Imagine that you pushed your work to the remote that is a shared
> repository, and then continued building on top of it, while others
> rewound the remote branch to eject your work, and your "git fetch"
> updated the remote-tracking branch. You'll be pretty much in the
> same situation you had in your reproduction recipe that rewound your
> own local branch that you used to build your derived work on and
> would lose your work the same way, if you do not notice that the
> remote branch has been rewound (and the fork-point heuristics chose
> a "wrong" commit from the reflog of your remote-tracking branch.
>
> Perhaps something along the lines of this (not even compile tested,
> though)... It might even be useful to show a shortlog between the
> .restrict_revision and .upstream, which is the list of commits that
> is potentially lost, but that might turn out to be excessively loud
> and noisy in the workflow of those who do benefit from the
> fork-point heuristics because their project rewinds branches too
> often and too wildly for them to manually keep track of. I dunno.
>
>
> builtin/rebase.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git c/builtin/rebase.c w/builtin/rebase.c
> index 50cb85751f..432a97e205 100644
> --- c/builtin/rebase.c
> +++ w/builtin/rebase.c
> @@ -1721,9 +1721,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (keep_base && options.reapply_cherry_picks)
> options.upstream = options.onto;
>
> - if (options.fork_point > 0)
> + if (options.fork_point > 0) {
> options.restrict_revision =
> get_fork_point(options.upstream_name, options.orig_head);
> + if (options.restrict_revision &&
> + options.restrict_revision != options.upstream)
> + warning(_("fork-point heuristics using %s from the reflog of %s"),
> + oid_to_hex(&options.restrict_revision->object.oid),
> + options.upstream_name);
> + }
>
> if (repo_read_index(the_repository) < 0)
> die(_("could not read index"));
>
^ 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