Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision
On 18/01/18 15:35, Johannes Schindelin wrote:
>
> In the upcoming commits, we will teach the sequencer to recreate merges.
> This will be done in a very different way from the unfortunate design of
> `git rebase --preserve-merges` (which does not allow for reordering
> commits, or changing the branch topology).
>
> The main idea is to introduce new todo list commands, to support
> labeling the current revision with a given name, resetting the current
> revision to a previous state, merging labeled revisions.
I think this would be a great improvement to rebase -i, thanks for
working on it.
> This idea was developed in Git for Windows' Git garden shears (that are
> used to maintain the "thicket of branches" on top of upstream Git), and
> this patch is part of the effort to make it available to a wider
> audience, as well as to make the entire process more robust (by
> implementing it in a safe and portable language rather than a Unix shell
> script).
>
> This commit implements the commands to label, and to reset to, given
> revisions. The syntax is:
>
> label
> reset
If I've understood the code below correctly then reset will clobber
untracked files, this is the opposite behaviour to what happens when
tries to checkout at the start of a rebase - then it will fail if
untracked files would be overwritten.
> As a convenience shortcut, also to improve readability of the generated
> todo list, a third command is introduced: bud. It simply resets to the
> "onto" revision, i.e. the commit onto which we currently rebase.
I found the whole bud business bewildering at first, reading the other
replies it seems I wasn't the only one to be befuddled by it. Having
seen an example I can see what it's trying to do but I still think it
adds more confusion than value.
> Internally, the `label ` command creates the ref
> `refs/rewritten/`. This makes it possible to work with the labeled
> revisions interactively, or in a scripted fashion (e.g. via the todo
> list command `exec`).
If a user has two work trees and runs a rebase in each with the same
label name, they'll clobber each other. I'd suggest storing them under
refs/rewritten/ instead. If the user
tries to rebase a second worktree with the same detached HEAD as an
existing rebase then refuse to start.
> Signed-off-by: Johannes Schindelin
> ---
> git-rebase--interactive.sh | 3 +
> sequencer.c| 181
> -
> 2 files changed, 180 insertions(+), 4 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index d47bd29593a..3d2cd19d65a 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -162,6 +162,9 @@ s, squash = use commit, but meld into previous commit
> f, fixup = like \"squash\", but discard this commit's log message
> x, exec = run command (the rest of the line) using shell
> d, drop = remove commit
> +l, label = label current HEAD with a name
> +t, reset = reset HEAD to a label
> +b, bud = reset HEAD to the revision labeled 'onto'
>
> These lines can be re-ordered; they are executed from top to bottom.
> " | git stripspace --comment-lines >>"$todo"
> diff --git a/sequencer.c b/sequencer.c
> index 4d3f60594cb..91cc55a002f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -21,6 +21,8 @@
> #include "log-tree.h"
> #include "wt-status.h"
> #include "hashmap.h"
> +#include "unpack-trees.h"
> +#include "worktree.h"
>
> #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>
> @@ -116,6 +118,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha,
> "rebase-merge/stopped-sha")
> static GIT_PATH_FUNC(rebase_path_rewritten_list,
> "rebase-merge/rewritten-list")
> static GIT_PATH_FUNC(rebase_path_rewritten_pending,
> "rebase-merge/rewritten-pending")
> +
> +/*
> + * The path of the file listing refs that need to be deleted after the rebase
> + * finishes. This is used by the `merge` command.
> + */
> +static GIT_PATH_FUNC(rebase_path_refs_to_delete,
> "rebase-merge/refs-to-delete")
> +
> /*
> * The following files are written by git-rebase just after parsing the
> * command-line (and are only consumed, not modified, by the sequencer).
> @@ -767,6 +776,9 @@ enum todo_command {
> TODO_SQUASH,
> /* commands that do something else than handling a single commit */
> TODO_EXEC,
> + TODO_LABEL,
> + TODO_RESET,
> + TODO_BUD,
> /* commands that do nothing but are counted for reporting progress */
> TODO_NOOP,
> TODO_DROP,
> @@ -785,6 +797,9 @@ static struct {
> { 'f', "fixup" },
> { 's', "squash" },
> { 'x', "exec" },
> + { 'l', "label" },
> + { 't', "reset" },
> + { 'b', "bud" },
> { 0, "noop" },
> { 'd', "drop" },
> { 0, NULL }
> @@ -1253,7 +1268,8 @@ static int parse_insn_line(struct todo_item *item,
> const char *bol, char *eol)
> if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
>
Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision
On 19/01/18 12:24, Phillip Wood wrote: > > On 18/01/18 15:35, Johannes Schindelin wrote: >> >> Internally, the `label ` command creates the ref >> `refs/rewritten/`. This makes it possible to work with the labeled >> revisions interactively, or in a scripted fashion (e.g. via the todo >> list command `exec`). > > If a user has two work trees and runs a rebase in each with the same > label name, they'll clobber each other. I'd suggest storing them under > refs/rewritten/ instead. If the user > tries to rebase a second worktree with the same detached HEAD as an > existing rebase then refuse to start. > Ah this isn't a concern after all as patch 5 makes refs/rewritten local to the worktree. Perhaps you could move that part of patch 5 here or add a note to the commit message that it will become worktree local later in the series Best Wishes Phillip
Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision
On Fri, Jan 19, 2018 at 10:55 AM, Phillip Wood wrote: > On 19/01/18 12:24, Phillip Wood wrote: >> >> On 18/01/18 15:35, Johannes Schindelin wrote: >>> >>> Internally, the `label ` command creates the ref >>> `refs/rewritten/`. This makes it possible to work with the labeled >>> revisions interactively, or in a scripted fashion (e.g. via the todo >>> list command `exec`). >> >> If a user has two work trees and runs a rebase in each with the same >> label name, they'll clobber each other. I'd suggest storing them under >> refs/rewritten/ instead. If the user >> tries to rebase a second worktree with the same detached HEAD as an >> existing rebase then refuse to start. >> > > Ah this isn't a concern after all as patch 5 makes refs/rewritten local > to the worktree. Perhaps you could move that part of patch 5 here or add > a note to the commit message that it will become worktree local later in > the series > > Best Wishes > > Phillip I'd rather it be included here as well. Thanks, Jake
Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision
Hi Phillip,
On Fri, 19 Jan 2018, Phillip Wood wrote:
>
> On 18/01/18 15:35, Johannes Schindelin wrote:
>
> > This idea was developed in Git for Windows' Git garden shears (that
> > are used to maintain the "thicket of branches" on top of upstream
> > Git), and this patch is part of the effort to make it available to a
> > wider audience, as well as to make the entire process more robust (by
> > implementing it in a safe and portable language rather than a Unix
> > shell script).
> >
> > This commit implements the commands to label, and to reset to, given
> > revisions. The syntax is:
> >
> > label
> > reset
>
> If I've understood the code below correctly then reset will clobber
> untracked files, this is the opposite behaviour to what happens when
> tries to checkout at the start of a rebase - then it will fail if
> untracked files would be overwritten.
This would be completely unintentional, I will verify that untracked files
are not clobbered.
However, in practice this should not happen because the intended use case
is for revisions to be labeled *before* checking them out at a later
stage. Therefore, the files that would be clobbered would already have
been tracked in the revision when it was labeled, and I do not quite see
how those files could become untracked without playing sloppy exec games
in between.
> > Internally, the `label ` command creates the ref
> > `refs/rewritten/`. This makes it possible to work with the labeled
> > revisions interactively, or in a scripted fashion (e.g. via the todo
> > list command `exec`).
>
> If a user has two work trees and runs a rebase in each with the same
> label name, they'll clobber each other. I'd suggest storing them under
> refs/rewritten/ instead. If the user
> tries to rebase a second worktree with the same detached HEAD as an
> existing rebase then refuse to start.
That is why a later patch marks those refs/rewritten/ refs as
worktree-local.
> > +static int do_label(const char *name, int len)
> > +{
> > + struct ref_store *refs = get_main_ref_store();
> > + struct ref_transaction *transaction;
> > + struct strbuf ref_name = STRBUF_INIT, err = STRBUF_INIT;
> > + struct strbuf msg = STRBUF_INIT;
> > + int ret = 0;
> > + struct object_id head_oid;
> > +
> > + strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
> > + strbuf_addf(&msg, "label '%.*s'", len, name);
>
> The other reflog messages below have a (rebase -i) prefix
Good point. I changed it to "rebase -i (label)".
> > + transaction = ref_store_transaction_begin(refs, &err);
> > + if (!transaction ||
> > + get_oid("HEAD", &head_oid) ||
> > + ref_transaction_update(transaction, ref_name.buf, &head_oid, NULL,
> > + 0, msg.buf, &err) < 0 ||
> > + ref_transaction_commit(transaction, &err)) {
> > + error("%s", err.buf);
>
> if get_oid() fails then err is empty so there wont be an message after
> the 'error: '
Yep, that would be nasty. Fixed.
> > +static int do_reset(const char *name, int len)
> > +{
> > + struct strbuf ref_name = STRBUF_INIT;
> > + struct object_id oid;
> > + struct lock_file lock = LOCK_INIT;
> > + struct tree_desc desc;
> > + struct tree *tree;
> > + struct unpack_trees_options opts;
> > + int ret = 0, i;
> > +
> > + if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> > + return -1;
> > +
> > + for (i = 0; i < len; i++)
> > + if (isspace(name[i]))
> > + len = i;
>
> If name starts with any white space then I think this effectively
> truncates name to a bunch of white space which doesn't sound right. I'm
> not sure how this is being called, but it might be better to clean up
> name when the to-do list is parsed instead.
The left-trimming of the name was already performed as part of the todo
list parsing.
And we are not really right-trimming here. We are splitting a line of the
form
reset
In fact, after reflecting about it, I changed the code so that it would
now even read:
reset #
So the code really is doing the intended thing here.
Ciao,
Dscho
Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision
Hi Jake, On Fri, 19 Jan 2018, Jacob Keller wrote: > On Fri, Jan 19, 2018 at 10:55 AM, Phillip Wood > wrote: > > On 19/01/18 12:24, Phillip Wood wrote: > >> > >> On 18/01/18 15:35, Johannes Schindelin wrote: > >>> > >>> Internally, the `label ` command creates the ref > >>> `refs/rewritten/`. This makes it possible to work with the labeled > >>> revisions interactively, or in a scripted fashion (e.g. via the todo > >>> list command `exec`). > >> > >> If a user has two work trees and runs a rebase in each with the same > >> label name, they'll clobber each other. I'd suggest storing them under > >> refs/rewritten/ instead. If the user > >> tries to rebase a second worktree with the same detached HEAD as an > >> existing rebase then refuse to start. > >> > > > > Ah this isn't a concern after all as patch 5 makes refs/rewritten local > > to the worktree. Perhaps you could move that part of patch 5 here or add > > a note to the commit message that it will become worktree local later in > > the series > > > > Best Wishes > > > > Phillip > > I'd rather it be included here as well. But it would have been really easy to overlook in here. I really want this to be a separate commit, also to have a chance to get this done *differently* if somebody comes up with a splendid idea how to do that (because hard-coding feels quite dirty). Ciao, Dscho
Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision
Hi, On Mon, 29 Jan 2018, Johannes Schindelin wrote: > On Fri, 19 Jan 2018, Jacob Keller wrote: > > > On Fri, Jan 19, 2018 at 10:55 AM, Phillip Wood > > wrote: > > > On 19/01/18 12:24, Phillip Wood wrote: > > >> > > >> On 18/01/18 15:35, Johannes Schindelin wrote: > > >>> > > >>> Internally, the `label ` command creates the ref > > >>> `refs/rewritten/`. This makes it possible to work with the labeled > > >>> revisions interactively, or in a scripted fashion (e.g. via the todo > > >>> list command `exec`). > > >> > > >> If a user has two work trees and runs a rebase in each with the same > > >> label name, they'll clobber each other. I'd suggest storing them under > > >> refs/rewritten/ instead. If the user > > >> tries to rebase a second worktree with the same detached HEAD as an > > >> existing rebase then refuse to start. > > >> > > > > > > Ah this isn't a concern after all as patch 5 makes refs/rewritten local > > > to the worktree. Perhaps you could move that part of patch 5 here or add > > > a note to the commit message that it will become worktree local later in > > > the series > > > > > > Best Wishes > > > > > > Phillip > > > > I'd rather it be included here as well. > > But it would have been really easy to overlook in here. I really want this > to be a separate commit, also to have a chance to get this done > *differently* if somebody comes up with a splendid idea how to do that > (because hard-coding feels quite dirty). BTW there is an additional good reason why the patch to make refs/rewritten/* worktree-local is so far away: that is the first time in the patch series when we can test this really effectively; at that stage we can easily just add to t3430 because all the building blocks for `rebase -i --recreate-merges` are in place. Ciao, Dscho
