[PATCH] rm: accept -R and --recursive in addition to -r
POSIX rm(1) accepts both -r and -R, so we accept -R here by analogy
with that and with commands like cp(1) + ls(1) + grep(1), where on
many or all platforms it’s the only way to recurse. For completeness
with GNU coreutils, we also accept --recursive here.
5c387428f10c2 introduces a mechanism that might have been nice to use
here (OPTION_ALIAS), but I didn’t use it because we would need to add
two different long options, and it’s primarily there to fix a problem
that won’t happen anyway unless there are two similar long options.
Signed-off-by: Delan Azabani
---
builtin/rm.c | 3 ++-
t/t3600-rm.sh | 20
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 19ce95a901..36c4cea256 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -242,7 +242,8 @@ static struct option builtin_rm_options[] = {
OPT__QUIET(&quiet, N_("do not list removed files")),
OPT_BOOL( 0 , "cached", &index_only, N_("only remove from the
index")),
OPT__FORCE(&force, N_("override the up-to-date check"),
PARSE_OPT_NOCOMPLETE),
- OPT_BOOL('r', NULL, &recursive, N_("allow recursive
removal")),
+ OPT_BOOL_F('R', NULL, &recursive, N_("allow recursive
removal"), PARSE_OPT_HIDDEN),
+ OPT_BOOL('r', "recursive", &recursive, N_("allow recursive
removal")),
OPT_BOOL( 0 , "ignore-unmatch", &ignore_unmatch,
N_("exit with a zero status even if nothing
matched")),
OPT_END(),
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 66282a720e..b2ddbba83c 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -212,6 +212,26 @@ test_expect_success 'Recursive with -r -f' '
test_path_is_missing frotz
'
+test_expect_success 'Recursive with -R' '
+ mkdir -p frotz &&
+ touch frotz/R &&
+ git add frotz &&
+ git commit -m R &&
+ git rm -R frotz &&
+ test_path_is_missing frotz/R &&
+ test_path_is_missing frotz
+'
+
+test_expect_success 'Recursive with --recursive' '
+ mkdir -p frotz &&
+ touch frotz/recursive &&
+ git add frotz &&
+ git commit -m recursive &&
+ git rm --recursive frotz &&
+ test_path_is_missing frotz/recursive &&
+ test_path_is_missing frotz
+'
+
test_expect_success 'Remove nonexistent file returns nonzero exit status' '
test_must_fail git rm nonexistent
'
--
2.19.2
Re: [PATCH 1/1] rebase -r: let `label` generate safer labels
I agree that the code locally was simple enough. Ultimately I feel that sanitizing and uniqueifying the label should probably be done closer together/at the same place. I'm just not familiar enough with the codebase to know a good place (if any) to move that to. Eventually though this would still need to be expanded further to protect against reserved filenames (e.g. NUL on windows). Although the behavior around these (espescially with file extensions like NUL.txt) become less reliable, and although they are much more unlikely to be encountered in practice, are still allowed by git as oneliners. On Tue, Sep 3, 2019 at 3:51 PM Junio C Hamano wrote: > > Johannes Schindelin writes: > > > If you care deeply about double dashes and leading dashes, how about > > this instead? > > > > char *from, *to; > > > > for (from = to = label.buf; *from; from++) > > if ((*from & 0x80) || isalnum(*from)) > > *(to++) = *from; > > /* avoid leading dash and double-dashes */ > > else if (to != label.buf && to[-1] != '-') > > *(to++) = '-'; > > strbuf_setlen(&label, to - label.buf); > > Simple enough and is a good change when judged locally. > > It still would cause readers to wonder if label_oid() later append > '-%d' to end up with double-dash near the end, etc., which made me > wonder if the resulting code becomes better if sanitization and > uniquefying are done at the same single place in the other message. -- Matthew Rogers
Re: [PATCH 1/1] rebase -r: let `label` generate safer labels
Johannes Schindelin writes: > If you care deeply about double dashes and leading dashes, how about > this instead? > > char *from, *to; > > for (from = to = label.buf; *from; from++) > if ((*from & 0x80) || isalnum(*from)) > *(to++) = *from; > /* avoid leading dash and double-dashes */ > else if (to != label.buf && to[-1] != '-') > *(to++) = '-'; > strbuf_setlen(&label, to - label.buf); Simple enough and is a good change when judged locally. It still would cause readers to wonder if label_oid() later append '-%d' to end up with double-dash near the end, etc., which made me wonder if the resulting code becomes better if sanitization and uniquefying are done at the same single place in the other message.
Re: [PATCH 1/1] rebase -r: let `label` generate safer labels
Johannes Schindelin writes: >> I'm sightly concerned that this opens the possibility for unexpected effects >> if two different labels get sanitized to the same string. I suspect it's >> unlikely to happen in practice but doing something like percent encoding >> non-alphanumeric characters would avoid the problem entirely. > > Oh, but we make sure that the labels are unique, via the `label_oid()` > function! Otherwise, we would not be able to label more than one merge > parent ;-) It somewhat feels suboptimal, from code followability's point of view, to have this "pre-sanitization" to replace isspace() to a dash, which is being extended to "all non-alnums", and the uniquefy of labels in label_oid(), in two separate places. I wonder if the resulting code becomes easier to follow and harder to introduce new bugs, if this part is made to just yield label.buf it obtained form the log message as-is and leave the munging to label_oid()?
Re: [PATCH 1/1] rebase -r: let `label` generate safer labels
Hi Junio, On Mon, 2 Sep 2019, Junio C Hamano wrote: > Phillip Wood writes: > > >>for (p1 = label.buf; *p1; p1++) > >> - if (isspace(*p1)) > >> + if (!(*p1 & 0x80) && !isalnum(*p1)) > >>*(char *)p1 = '-'; > > > > I'm sightly concerned that this opens the possibility for unexpected > > effects if two different labels get sanitized to the same string. I > > suspect it's unlikely to happen in practice but doing something like > > percent encoding non-alphanumeric characters would avoid the problem > > entirely. > > I'd rather see 'x' used instead of '-' (double-or-more dashes and > leading dash in refnames may currently be allowed but double-or-more > exes and leading ex would be much more likely to stay valid) if we > just want to redact invalid characters. Hmm. Let's take a concrete example from the VFS for Git fork: Merge pull request #160: trace2:gvfs:experiment Add experimental regions and data events to help diagnose checkout and reset perf problems (Yes, we have some quite verbose merge commits, with very, very long onelines. Not a good practice, we stopped doing it, but it was well within what Git allows.) And now use dashes to encode all white-space: Merge-pull-request-#160:-trace2:gvfs:experiment-Add-experimental-regions-and-data-events-to-help-diagnose-checkout-and-reset-perf-problems Pretty long, but looks okay. Of course, it does not work, because colons. So here is the label with Matt's patch: Merge-pull-request--160--trace2-gvfs-experiment-Add-experimental-regions-and-data-events-to-help-diagnose-checkout-and-reset-perf-problems And here is the label with your proposed xs. Mergexpullxrequestxx160xxtrace2xgvfsxexperimentxAddxexperimentalxregionsxandxdataxeventsxtoxhelpxdiagnosexcheckoutxandxresetxperfxproblems I cannot speak for you, of course, but I can speak for myself: this is not only way too reminiscent of xoxoxothxbye, but it is also really, totally unreadable. If you care deeply about double dashes and leading dashes, how about this instead? char *from, *to; for (from = to = label.buf; *from; from++) if ((*from & 0x80) || isalnum(*from)) *(to++) = *from; /* avoid leading dash and double-dashes */ else if (to != label.buf && to[-1] != '-') *(to++) = '-'; strbuf_setlen(&label, to - label.buf); That would result in Merge-pull-request-160-trace2-gvfs-experiment-Add-experimental-regions-and-data-events-to-help-diagnose-checkout-and-reset-perf-problems > I see there are "lets make sure it is unique by suffixing "-%d" in > other codepaths; would that help if this piece of code yields a > label that is not unique? I'm way ahead of you. The sequencer already goes out of its way to guarantee the uniqueness of the labels (it's part of the design, as applied in 1644c73c6d4 (rebase-helper --make-script: introduce a flag to rebase merges, 2018-04-25)). The patch you are looking at in this thread is only concerned about the initial phase, `label_oid()` does a lot more. Not only does it make the label unique (case-insensitively!), it also prevents it from looking like a full 40-hex digit SHA-1, so that we can guarantee that unique abbreviations of commit hashes will work as labels, too. Ciao, Dscho
Re: [PATCH 1/1] rebase -r: let `label` generate safer labels
On 02/09/2019 22:47, Matt Rogers wrote: I can redo the commit, I had thought that I had previously fixed the author but I guess I was mistaken. As for issues with non utf-8 encodings, I don't know of any simple way to check for those except for restricting to asci alphanumeric characters If I read the Wikipedia article [1] about the utf-8 design choices it is pretty reasonable and robust most of the time, though that maybe a part one way trapdoor. Also the code given doesn't resolve onelines that consist only of restricted file names (e.g. COM, NUL, etc. On windows) It maybe that the rebase doc may need (if it happens) a short comment warning of that. Also need to check if the `label_oid()` function actually makes the label distinct, hence prevents such labels from being used as such a restricted file name - i.e. does it include the oid element. Ultimately the label could be tweaked to have say the 4char prefix to fool the Windows 'starts with' name detection - which assumes I understand how some of those bad filenames are detected... On Mon, Sep 2, 2019, 5:24 PM Philip Oakley wrote: On 02/09/2019 19:29, Junio C Hamano wrote: > I see there are "lets make sure it is unique by suffixing "-%d" in > other codepaths; would that help if this piece of code yields a > label that is not unique? maybe use a trailing 4 characters of the oid to get a reasonably unique label? Oh, just seen dscho's "we make sure that the labels are unique, via the `label_oid()` function!", maybe needs mentioning in the commit message if re-rolled. Philip [1] https://en.wikipedia.org/wiki/UTF-8#Description
Re: [PATCH 1/1] rebase -r: let `label` generate safer labels
On 02/09/2019 19:29, Junio C Hamano wrote: I see there are "lets make sure it is unique by suffixing "-%d" in other codepaths; would that help if this piece of code yields a label that is not unique? maybe use a trailing 4 characters of the oid to get a reasonably unique label? Oh, just seen dscho's "we make sure that the labels are unique, via the `label_oid()` function!", maybe needs mentioning in the commit message if re-rolled. Philip
Re: [PATCH 1/1] rebase -r: let `label` generate safer labels
On 2019-09-02 at 18:29:56, Junio C Hamano wrote: > Phillip Wood writes: > > > Being picking I'll point out that ':' is not a valid in refs > > either. Looking at > > https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I > > think only " and | are not allowed on NTFS/FAT but are valid in refs > > (see the man page for git check-ref-format for all the details). So > > the main limitation is actually what git allows in refs. > > Yeah, trying to use the contents of the log message without > sufficient sanitization is looking for trouble. > > >>for (p1 = label.buf; *p1; p1++) > >> - if (isspace(*p1)) > >> + if (!(*p1 & 0x80) && !isalnum(*p1)) > >>*(char *)p1 = '-'; While we're thinking of things that could go wrong, note that it's also possible for the commit message to contain non-UTF-8 characters (if the user is using a non-UTF-8 encoding), which will cause sadness on Windows and macOS. Non-Mac Unix systems aren't a problem here, but then again, they aren't the reason for this patch. > > I'm sightly concerned that this opens the possibility for unexpected > > effects if two different labels get sanitized to the same string. I > > suspect it's unlikely to happen in practice but doing something like > > percent encoding non-alphanumeric characters would avoid the problem > > entirely. > > I see there are "lets make sure it is unique by suffixing "-%d" in > other codepaths; would that help if this piece of code yields a > label that is not unique? I was thinking the same thing. Since we're being much less lenient on what's allowed (which is fine), we're at increased risk for collision. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH 1/1] rebase -r: let `label` generate safer labels
Hi Phillip, On Mon, 2 Sep 2019, Phillip Wood wrote: > This is definitely worth fixing, I've got a couple of comments below > > On 02/09/2019 15:01, Matt R via GitGitGadget wrote: > > From: Matt R I just noticed that the surname is abbreviated. The full name of the author is "Matt Rogers". (Matt, Git uses the Signed-off-by: lines as some sort of legally-binding assurance that you are free to submit these changes under the GPLv2, so your full name is kinda required.) > > The `label` todo command in interactive rebases creates temporary refs > > in the `refs/rewritten/` namespace. These refs are stored as loose refs, > > i.e. as files in `.git/refs/rewritten/`, therefore they have to conform > > with file name limitations on the current filesystem. > > > > This poses a problem in particular on NTFS/FAT, where e.g. the colon > > character is not a valid part of a file name. > > Being picking I'll point out that ':' is not a valid in refs either. True, but that was not the primary concern here ;-) > Looking at > https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I > think only " and | are not allowed on NTFS/FAT but are valid in refs > (see the man page for git check-ref-format for all the details). So > the main limitation is actually what git allows in refs. Right. And this example shows that we really need to be a bit more conservative than just disallowing characters that would not be allowed in refs. I think it is more important to keep in mind the vagaries of various current and future filesystems to justify the change in this patch. > > Let's safeguard against this by replacing not only white-space > > characters by dashes, but all non-alpha-numeric ones. > > > > However, we exempt non-ASCII UTF-8 characters from that, as it should be > > quite possible to reflect branch names such as `↯↯↯` in refs/file names. > > > > Signed-off-by: Matthew Rogers > > Signed-off-by: Johannes Schindelin > > --- > > sequencer.c | 12 +++- > > t/t3430-rebase-merges.sh | 5 + > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/sequencer.c b/sequencer.c > > index 34ebf8ed94..23f4a0876a 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -4635,8 +4635,18 @@ static int make_script_with_merges(struct > > pretty_print_context *pp, > > else > > strbuf_addbuf(&label, &oneline); > > + /* > > +* Sanitize labels by replacing non-alpha-numeric characters > > +* (including white-space ones) by dashes, as they might be > > +* illegal in file names (and hence in ref names). > > +* > > +* Note that we retain non-ASCII UTF-8 characters (identified > > +* via the most significant bit). They should be all > > acceptable > > +* in file names. We do not validate the UTF-8 here, that's > > not > > +* the job of this function. > > +*/ > > for (p1 = label.buf; *p1; p1++) > > - if (isspace(*p1)) > > + if (!(*p1 & 0x80) && !isalnum(*p1)) > > *(char *)p1 = '-'; > > I'm sightly concerned that this opens the possibility for unexpected effects > if two different labels get sanitized to the same string. I suspect it's > unlikely to happen in practice but doing something like percent encoding > non-alphanumeric characters would avoid the problem entirely. Oh, but we make sure that the labels are unique, via the `label_oid()` function! Otherwise, we would not be able to label more than one merge parent ;-) Ciao, Dscho
Re: [PATCH 1/1] rebase -r: let `label` generate safer labels
Phillip Wood writes: > Being picking I'll point out that ':' is not a valid in refs > either. Looking at > https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I > think only " and | are not allowed on NTFS/FAT but are valid in refs > (see the man page for git check-ref-format for all the details). So > the main limitation is actually what git allows in refs. Yeah, trying to use the contents of the log message without sufficient sanitization is looking for trouble. >> for (p1 = label.buf; *p1; p1++) >> -if (isspace(*p1)) >> +if (!(*p1 & 0x80) && !isalnum(*p1)) >> *(char *)p1 = '-'; > > I'm sightly concerned that this opens the possibility for unexpected > effects if two different labels get sanitized to the same string. I > suspect it's unlikely to happen in practice but doing something like > percent encoding non-alphanumeric characters would avoid the problem > entirely. I'd rather see 'x' used instead of '-' (double-or-more dashes and leading dash in refnames may currently be allowed but double-or-more exes and leading ex would be much more likely to stay valid) if we just want to redact invalid characters. I see there are "lets make sure it is unique by suffixing "-%d" in other codepaths; would that help if this piece of code yields a label that is not unique?
Re: [PATCH 1/1] rebase -r: let `label` generate safer labels
Hi Matt This is definitely worth fixing, I've got a couple of comments below On 02/09/2019 15:01, Matt R via GitGitGadget wrote: From: Matt R The `label` todo command in interactive rebases creates temporary refs in the `refs/rewritten/` namespace. These refs are stored as loose refs, i.e. as files in `.git/refs/rewritten/`, therefore they have to conform with file name limitations on the current filesystem. This poses a problem in particular on NTFS/FAT, where e.g. the colon character is not a valid part of a file name. Being picking I'll point out that ':' is not a valid in refs either. Looking at https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I think only " and | are not allowed on NTFS/FAT but are valid in refs (see the man page for git check-ref-format for all the details). So the main limitation is actually what git allows in refs. Let's safeguard against this by replacing not only white-space characters by dashes, but all non-alpha-numeric ones. However, we exempt non-ASCII UTF-8 characters from that, as it should be quite possible to reflect branch names such as `↯↯↯` in refs/file names. Signed-off-by: Matthew Rogers Signed-off-by: Johannes Schindelin --- sequencer.c | 12 +++- t/t3430-rebase-merges.sh | 5 + 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 34ebf8ed94..23f4a0876a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4635,8 +4635,18 @@ static int make_script_with_merges(struct pretty_print_context *pp, else strbuf_addbuf(&label, &oneline); + /* +* Sanitize labels by replacing non-alpha-numeric characters +* (including white-space ones) by dashes, as they might be +* illegal in file names (and hence in ref names). +* +* Note that we retain non-ASCII UTF-8 characters (identified +* via the most significant bit). They should be all acceptable +* in file names. We do not validate the UTF-8 here, that's not +* the job of this function. +*/ for (p1 = label.buf; *p1; p1++) - if (isspace(*p1)) + if (!(*p1 & 0x80) && !isalnum(*p1)) *(char *)p1 = '-'; I'm sightly concerned that this opens the possibility for unexpected effects if two different labels get sanitized to the same string. I suspect it's unlikely to happen in practice but doing something like percent encoding non-alphanumeric characters would avoid the problem entirely. Best Wishes Phillip strbuf_reset(&buf); diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 7b6c4847ad..737396f944 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -441,4 +441,9 @@ test_expect_success '--continue after resolving conflicts after a merge' ' test_path_is_missing .git/MERGE_HEAD ' +test_expect_success '--rebase-merges with commit that can generate bad characters for filename' ' + git checkout -b colon-in-label E && + git merge -m "colon: this should work" G && + git rebase --rebase-merges --force-rebase E +' test_done
[PATCH 1/1] rebase -r: let `label` generate safer labels
From: Matt R The `label` todo command in interactive rebases creates temporary refs in the `refs/rewritten/` namespace. These refs are stored as loose refs, i.e. as files in `.git/refs/rewritten/`, therefore they have to conform with file name limitations on the current filesystem. This poses a problem in particular on NTFS/FAT, where e.g. the colon character is not a valid part of a file name. Let's safeguard against this by replacing not only white-space characters by dashes, but all non-alpha-numeric ones. However, we exempt non-ASCII UTF-8 characters from that, as it should be quite possible to reflect branch names such as `↯↯↯` in refs/file names. Signed-off-by: Matthew Rogers Signed-off-by: Johannes Schindelin --- sequencer.c | 12 +++- t/t3430-rebase-merges.sh | 5 + 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 34ebf8ed94..23f4a0876a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4635,8 +4635,18 @@ static int make_script_with_merges(struct pretty_print_context *pp, else strbuf_addbuf(&label, &oneline); + /* +* Sanitize labels by replacing non-alpha-numeric characters +* (including white-space ones) by dashes, as they might be +* illegal in file names (and hence in ref names). +* +* Note that we retain non-ASCII UTF-8 characters (identified +* via the most significant bit). They should be all acceptable +* in file names. We do not validate the UTF-8 here, that's not +* the job of this function. +*/ for (p1 = label.buf; *p1; p1++) - if (isspace(*p1)) + if (!(*p1 & 0x80) && !isalnum(*p1)) *(char *)p1 = '-'; strbuf_reset(&buf); diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 7b6c4847ad..737396f944 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -441,4 +441,9 @@ test_expect_success '--continue after resolving conflicts after a merge' ' test_path_is_missing .git/MERGE_HEAD ' +test_expect_success '--rebase-merges with commit that can generate bad characters for filename' ' + git checkout -b colon-in-label E && + git merge -m "colon: this should work" G && + git rebase --rebase-merges --force-rebase E +' test_done -- gitgitgadget
[PATCH v2 16/16] rebase -r: do not (re-)generate root commits with `--root` *and* `--onto`
From: Johannes Schindelin
When rebasing a complete commit history onto a given commit, it is
pretty obvious that the root commits should be rebased on top of said
given commit.
To test this, let's kill two birds with one stone and add a test case to
t3427-rebase-subtree.sh that not only demonstrates that this works, but
also that `git rebase -r` works with merge strategies now.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 7 +--
sequencer.c | 4 +++-
sequencer.h | 6 ++
t/t3427-rebase-subtree.sh | 11 +++
4 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 625f50c637..ee2bc8b032 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -62,7 +62,7 @@ struct rebase_options {
const char *onto_name;
const char *revisions;
const char *switch_to;
- int root;
+ int root, root_with_onto;
struct object_id *squash_onto;
struct commit *restrict_revision;
int dont_finish_rebase;
@@ -374,6 +374,7 @@ static int run_rebase_interactive(struct rebase_options
*opts,
flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
flags |= opts->rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
+ flags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;
flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
switch (command) {
@@ -1841,7 +1842,9 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
options.squash_onto = &squash_onto;
options.onto_name = squash_onto_name =
xstrdup(oid_to_hex(&squash_onto));
- }
+ } else
+ options.root_with_onto = 1;
+
options.upstream_name = NULL;
options.upstream = NULL;
if (argc > 1)
diff --git a/sequencer.c b/sequencer.c
index d228448cd8..ca119c84e5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4440,6 +4440,7 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
{
int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
+ int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
struct strbuf label = STRBUF_INIT;
struct commit_list *commits = NULL, **tail = &commits, *iter;
@@ -4606,7 +4607,8 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
if (!commit)
strbuf_addf(out, "%s %s\n", cmd_reset,
- rebase_cousins ? "onto" : "[new root]");
+ rebase_cousins || root_with_onto ?
+ "onto" : "[new root]");
else {
const char *to = NULL;
diff --git a/sequencer.h b/sequencer.h
index 0c494b83d4..d506081d3c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -142,6 +142,12 @@ int sequencer_remove_state(struct replay_opts *opts);
*/
#define TODO_LIST_REBASE_COUSINS (1U << 4)
#define TODO_LIST_APPEND_TODO_HELP (1U << 5)
+/*
+ * When generating a script that rebases merges with `--root` *and* with
+ * `--onto`, we do not want to re-generate the root commits.
+ */
+#define TODO_LIST_ROOT_WITH_ONTO (1U << 6)
+
int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
const char **argv, unsigned flags);
diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
index 7a37235768..39e348de16 100755
--- a/t/t3427-rebase-subtree.sh
+++ b/t/t3427-rebase-subtree.sh
@@ -93,4 +93,15 @@ test_expect_success 'Rebase -Xsubtree --keep-empty --onto
commit' '
verbose test "$(commit_message HEAD)" = "Empty commit"
'
+test_expect_success 'Rebase -Xsubtree --keep-empty --rebase-merges --onto
commit' '
+ reset_rebase &&
+ git checkout -b rebase-merges-onto to-rebase &&
+ test_must_fail git rebase -Xsubtree=files_subtree --keep-empty
--rebase-merges --onto files-master --root &&
+ : first pick results in no changes &&
+ git rebase --continue &&
+ verbose test "$(commit_message HEAD~2)" = "master4" &&
+ verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
+ verbose test "$(commit_message HEAD)" = "Empty commit"
+'
+
test_done
--
gitgitgadget
[PATCH v2 13/16] rebase -r: support merge strategies other than `recursive`
From: Johannes Schindelin
We already support merge strategies in the sequencer, but only for
`pick` commands.
With this commit, we now also support them in `merge` commands. The
approach is simple: if any merge strategy option is specified, or if any
merge strategy other than `recursive` is specified, we simply spawn the
`git merge` command. Otherwise, we handle the merge in-process just as
before.
Signed-off-by: Johannes Schindelin
---
Documentation/git-rebase.txt | 2 --
builtin/rebase.c | 9 -
sequencer.c| 14 --
t/t3422-rebase-incompatible-options.sh | 10 --
t/t3430-rebase-merges.sh | 21 +
5 files changed, 33 insertions(+), 23 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 5e4e927647..bc620c44e9 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -543,8 +543,6 @@ In addition, the following pairs of options are
incompatible:
* --preserve-merges and --interactive
* --preserve-merges and --signoff
* --preserve-merges and --rebase-merges
- * --rebase-merges and --strategy
- * --rebase-merges and --strategy-option
BEHAVIORAL DIFFERENCES
---
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 74a60e8c83..625f50c637 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1811,15 +1811,6 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
"'--reschedule-failed-exec'"));
}
- if (options.rebase_merges) {
- if (strategy_options.nr)
- die(_("cannot combine '--rebase-merges' with "
- "'--strategy-option'"));
- if (options.strategy)
- die(_("cannot combine '--rebase-merges' with "
- "'--strategy'"));
- }
-
if (!options.root) {
if (argc < 1) {
struct branch *branch;
diff --git a/sequencer.c b/sequencer.c
index 334de14542..d228448cd8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3256,6 +3256,9 @@ static int do_merge(struct repository *r,
struct commit *head_commit, *merge_commit, *i;
struct commit_list *bases, *j, *reversed = NULL;
struct commit_list *to_merge = NULL, **tail = &to_merge;
+ const char *strategy = !opts->xopts_nr &&
+ (!opts->strategy || !strcmp(opts->strategy, "recursive")) ?
+ NULL : opts->strategy;
struct merge_options o;
int merge_arg_len, oneline_offset, can_fast_forward, ret, k;
static struct lock_file lock;
@@ -3404,7 +3407,7 @@ static int do_merge(struct repository *r,
goto leave_merge;
}
- if (to_merge->next) {
+ if (strategy || to_merge->next) {
/* Octopus merge */
struct child_process cmd = CHILD_PROCESS_INIT;
@@ -3418,7 +3421,14 @@ static int do_merge(struct repository *r,
cmd.git_cmd = 1;
argv_array_push(&cmd.args, "merge");
argv_array_push(&cmd.args, "-s");
- argv_array_push(&cmd.args, "octopus");
+ if (!strategy)
+ argv_array_push(&cmd.args, "octopus");
+ else {
+ argv_array_push(&cmd.args, strategy);
+ for (k = 0; k < opts->xopts_nr; k++)
+ argv_array_pushf(&cmd.args,
+"-X%s", opts->xopts[k]);
+ }
argv_array_push(&cmd.args, "--no-edit");
argv_array_push(&cmd.args, "--no-ff");
argv_array_push(&cmd.args, "--no-log");
diff --git a/t/t3422-rebase-incompatible-options.sh
b/t/t3422-rebase-incompatible-options.sh
index a5868ea152..50e7960702 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -76,14 +76,4 @@ test_expect_success REBASE_P \
test_must_fail git rebase --preserve-merges --rebase-merges A
'
-test_expect_success '--rebase-merges incompatible with --strategy' '
- git checkout B^0 &&
- test_must_fail git rebase --rebase-merges -s resolve A
-'
-
-test_expect_success '--rebase-merges incompatible with --strategy-option' '
- git checkout B^0 &&
- test_must_fail git rebase --rebase-merges -Xignore-space-change A
-'
-
test_done
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 42ba5b9f09..8ea6ff3548 100755
--- a/t/t3430-rebase-merges.sh
[PATCH v2 15/16] t3418: test `rebase -r` with merge strategies
From: Johannes Schindelin There is a test case in this script that verifies that `git rebase --preserve-merges` works all right with non-default merge strategies or non-default merge strategy options. Now that `git rebase --rebase-merges` learned about merge strategies, let's copy-edit this test case to verify that that works as intended, too. Signed-off-by: Johannes Schindelin --- t/t3418-rebase-continue.sh | 14 ++ 1 file changed, 14 insertions(+) diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index bdaa511bb0..fbf9addfd1 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -120,6 +120,20 @@ test_expect_success REBASE_P 'rebase passes merge strategy options correctly' ' git rebase --continue ' +test_expect_success 'rebase -r passes merge strategy options correctly' ' + rm -fr .git/rebase-* && + git reset --hard commit-new-file-F3-on-topic-branch && + test_commit merge-theirs && + git reset --hard HEAD^ && + test_commit some-other-commit && + test_tick && + git merge --no-ff merge-theirs && + FAKE_LINES="1 3 edit 4 5 7 8 9" git rebase -i -f -r -m \ + -s recursive --strategy-option=theirs HEAD~2 && + test_commit force-change-ours && + git rebase --continue +' + test_expect_success '--skip after failed fixup cleans commit message' ' test_when_finished "test_might_fail git rebase --abort" && git checkout -b with-conflicting-fixup && -- gitgitgadget
[PATCH v2 00/16] rebase -r: support merge strategies other than recursive
This is the most notable shortcoming that --rebase-merges has, still, relative to --preserve-merges' capabilities: it does not support passing custom merge strategies or custom merge strategy options. Let's fix this. While working on this patch series, of course I tried to copy-edit the test cases we have, to cover --preserve-merges' support for merge strategies. Oh my, did I regret this decision as soon as my eyes set sight on t3427-rebase-subtree.sh! At first I tried my best to make heads or tails of t3427, for way too long. In the end the only way to understand what the heck it tries to do was to actually fix it. That's why this patch series looks as if it focuses on t3427 rather than on adding support for custom merge strategies to the --rebase-merges mode. As a consolation to myself, this work was actually worth it, surprising as that may look. Not only is t3427 now really easy to understand, adding that test case for --rebase-merges -Xsubtree tickled the sequencer enough to reveal a long-standing bug: the --onto option was simply ignored when passed together with --rebase-merges and --root. For good measure, this patch series addresses this bug, too. Changes since v1: * Rebased to the current js/rebase-cleanup: that branch itself was rebased, and as a consequence, one already-applied patch was dropped from this patch series. * Forward-fixed the fakesha handling, just in case that anybody wants to use it with a regular pick command in the future (thanks, brian). Johannes Schindelin (16): Drop unused git-rebase--am.sh t3400: stop referring to the scripted rebase .gitignore: there is no longer a built-in `git-rebase--interactive` sequencer: the `am` and `rebase--interactive` scripts are gone rebase: fold git-rebase--common into the -p backend t3427: add a clarifying comment t3427: simplify the `setup` test case significantly t3427: move the `filter-branch` invocation into the `setup` case t3427: condense the unnecessarily repetitive test cases into three t3427: fix erroneous assumption t3427: accommodate for the `rebase --merge` backend having been replaced t3427: fix another incorrect assumption rebase -r: support merge strategies other than `recursive` t/lib-rebase: prepare for testing `git rebase --rebase-merges` t3418: test `rebase -r` with merge strategies rebase -r: do not (re-)generate root commits with `--root` *and* `--onto` .gitignore | 3 - Documentation/git-rebase.txt | 2 - Makefile | 2 - builtin/rebase.c | 23 +--- git-rebase--am.sh | 85 -- git-rebase--common.sh | 69 --- git-rebase--preserve-merges.sh | 55 + sequencer.c| 20 +++- sequencer.h| 6 + t/lib-rebase.sh| 9 +- t/t3400-rebase.sh | 2 +- t/t3418-rebase-continue.sh | 14 +++ t/t3422-rebase-incompatible-options.sh | 10 -- t/t3427-rebase-subtree.sh | 155 +++-- t/t3430-rebase-merges.sh | 21 15 files changed, 193 insertions(+), 283 deletions(-) delete mode 100644 git-rebase--am.sh delete mode 100644 git-rebase--common.sh base-commit: 80dfc9242ebaba357ffedececd88641a1a752411 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-294%2Fdscho%2Frebase-r-with-strategies-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-294/dscho/rebase-r-with-strategies-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/294 Range-diff vs v1: -: -- > 1: e0645b3ad7 Drop unused git-rebase--am.sh -: -- > 2: 6e8be7d873 t3400: stop referring to the scripted rebase -: -- > 3: 9e00e66dc7 .gitignore: there is no longer a built-in `git-rebase--interactive` -: -- > 4: db9ec248e1 sequencer: the `am` and `rebase--interactive` scripts are gone -: -- > 5: 38c8e3e284 rebase: fold git-rebase--common into the -p backend 1: 05be92d921 = 6: b3daf078e8 t3427: add a clarifying comment 2: df096e054d = 7: c168c4499b t3427: simplify the `setup` test case significantly 3: a3944c5480 ! 8: 138ff362fb t3427: move the `filter-branch` invocation into the `setup` case @@ -29,7 +29,8 @@ ' # FAILURE: Does not preserve master4. - test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 4' ' + test_expect_failure REBASE_P \ + 'Rebase -Xsubtree --preserve-merges --onto commit 4' ' reset_rebase && - git checkout -b rebase-preserve-merges-4 master && - git filter-branch --prune-empty -f --subdirectory-filter files_subtree && @@ -39,8 +40,8 @@ verbose test "$(commit_mess
Re: [PATCH 00/12] rebase -r: support merge strategies other than recursive
On 2019-07-25 at 10:11:14, Johannes Schindelin via GitGitGadget wrote: > This is the most notable shortcoming that --rebase-merges has, still, > relative to --preserve-merges' capabilities: it does not support passing > custom merge strategies or custom merge strategy options. > > Let's fix this. This looks like a great improvement. I'm glad to see --rebase-merges gaining feature parity with --preserve-merges. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH 00/12] rebase -r: support merge strategies other than recursive
"Johannes Schindelin via GitGitGadget" writes: > ... As a consolation to myself, this work was actually worth it, surprising as > that may look. Not only is t3427 now really easy to understand, adding that > test case for --rebase-merges -Xsubtree tickled the sequencer enough to > reveal a long-standing bug: the --onto option was simply ignored when passed > together with --rebase-merges and --root. For good measure, this patch > series addresses this bug, too. Very nice. > base-commit: 082ef75b7bfc90ac236afbb857a9552a026832b8 > Published-As: > https://github.com/gitgitgadget/git/releases/tag/pr-294%2Fdscho%2Frebase-r-with-strategies-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git > pr-294/dscho/rebase-r-with-strategies-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/294
[PATCH 09/12] rebase -r: support merge strategies other than `recursive`
From: Johannes Schindelin
We already support merge strategies in the sequencer, but only for
`pick` commands.
With this commit, we now also support them in `merge` commands. The
approach is simple: if any merge strategy option is specified, or if any
merge strategy other than `recursive` is specified, we simply spawn the
`git merge` command. Otherwise, we handle the merge in-process just as
before.
Signed-off-by: Johannes Schindelin
---
Documentation/git-rebase.txt | 2 --
builtin/rebase.c | 9 -
sequencer.c| 14 --
t/t3422-rebase-incompatible-options.sh | 10 --
t/t3430-rebase-merges.sh | 21 +
5 files changed, 33 insertions(+), 23 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f5e6ae3907..f67f96425c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -543,8 +543,6 @@ In addition, the following pairs of options are
incompatible:
* --preserve-merges and --interactive
* --preserve-merges and --signoff
* --preserve-merges and --rebase-merges
- * --rebase-merges and --strategy
- * --rebase-merges and --strategy-option
BEHAVIORAL DIFFERENCES
---
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 9c52144fc4..c1ea617125 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1815,15 +1815,6 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
"'--reschedule-failed-exec'"));
}
- if (options.rebase_merges) {
- if (strategy_options.nr)
- die(_("cannot combine '--rebase-merges' with "
- "'--strategy-option'"));
- if (options.strategy)
- die(_("cannot combine '--rebase-merges' with "
- "'--strategy'"));
- }
-
if (!options.root) {
if (argc < 1) {
struct branch *branch;
diff --git a/sequencer.c b/sequencer.c
index 334de14542..d228448cd8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3256,6 +3256,9 @@ static int do_merge(struct repository *r,
struct commit *head_commit, *merge_commit, *i;
struct commit_list *bases, *j, *reversed = NULL;
struct commit_list *to_merge = NULL, **tail = &to_merge;
+ const char *strategy = !opts->xopts_nr &&
+ (!opts->strategy || !strcmp(opts->strategy, "recursive")) ?
+ NULL : opts->strategy;
struct merge_options o;
int merge_arg_len, oneline_offset, can_fast_forward, ret, k;
static struct lock_file lock;
@@ -3404,7 +3407,7 @@ static int do_merge(struct repository *r,
goto leave_merge;
}
- if (to_merge->next) {
+ if (strategy || to_merge->next) {
/* Octopus merge */
struct child_process cmd = CHILD_PROCESS_INIT;
@@ -3418,7 +3421,14 @@ static int do_merge(struct repository *r,
cmd.git_cmd = 1;
argv_array_push(&cmd.args, "merge");
argv_array_push(&cmd.args, "-s");
- argv_array_push(&cmd.args, "octopus");
+ if (!strategy)
+ argv_array_push(&cmd.args, "octopus");
+ else {
+ argv_array_push(&cmd.args, strategy);
+ for (k = 0; k < opts->xopts_nr; k++)
+ argv_array_pushf(&cmd.args,
+"-X%s", opts->xopts[k]);
+ }
argv_array_push(&cmd.args, "--no-edit");
argv_array_push(&cmd.args, "--no-ff");
argv_array_push(&cmd.args, "--no-log");
diff --git a/t/t3422-rebase-incompatible-options.sh
b/t/t3422-rebase-incompatible-options.sh
index bb78a6ec86..596caf168a 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -75,14 +75,4 @@ test_expect_success '--preserve-merges incompatible with
--rebase-merges' '
test_must_fail git rebase --preserve-merges --rebase-merges A
'
-test_expect_success '--rebase-merges incompatible with --strategy' '
- git checkout B^0 &&
- test_must_fail git rebase --rebase-merges -s resolve A
-'
-
-test_expect_success '--rebase-merges incompatible with --strategy-option' '
- git checkout B^0 &&
- test_must_fail git rebase --rebase-merges -Xignore-space-change A
-'
-
test_done
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
[PATCH 12/12] rebase -r: do not (re-)generate root commits with `--root` *and* `--onto`
From: Johannes Schindelin
When rebasing a complete commit history onto a given commit, it is
pretty obvious that the root commits should be rebased on top of said
given commit.
To test this, let's kill two birds with one stone and add a test case to
t3427-rebase-subtree.sh that not only demonstrates that this works, but
also that `git rebase -r` works with merge strategies now.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 7 +--
sequencer.c | 4 +++-
sequencer.h | 6 ++
t/t3427-rebase-subtree.sh | 11 +++
4 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index c1ea617125..6a789c4421 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -62,7 +62,7 @@ struct rebase_options {
const char *onto_name;
const char *revisions;
const char *switch_to;
- int root;
+ int root, root_with_onto;
struct object_id *squash_onto;
struct commit *restrict_revision;
int dont_finish_rebase;
@@ -374,6 +374,7 @@ static int run_rebase_interactive(struct rebase_options
*opts,
flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
flags |= opts->rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
+ flags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;
flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
switch (command) {
@@ -1845,7 +1846,9 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
options.squash_onto = &squash_onto;
options.onto_name = squash_onto_name =
xstrdup(oid_to_hex(&squash_onto));
- }
+ } else
+ options.root_with_onto = 1;
+
options.upstream_name = NULL;
options.upstream = NULL;
if (argc > 1)
diff --git a/sequencer.c b/sequencer.c
index d228448cd8..ca119c84e5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4440,6 +4440,7 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
{
int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
+ int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
struct strbuf label = STRBUF_INIT;
struct commit_list *commits = NULL, **tail = &commits, *iter;
@@ -4606,7 +4607,8 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
if (!commit)
strbuf_addf(out, "%s %s\n", cmd_reset,
- rebase_cousins ? "onto" : "[new root]");
+ rebase_cousins || root_with_onto ?
+ "onto" : "[new root]");
else {
const char *to = NULL;
diff --git a/sequencer.h b/sequencer.h
index 0c494b83d4..d506081d3c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -142,6 +142,12 @@ int sequencer_remove_state(struct replay_opts *opts);
*/
#define TODO_LIST_REBASE_COUSINS (1U << 4)
#define TODO_LIST_APPEND_TODO_HELP (1U << 5)
+/*
+ * When generating a script that rebases merges with `--root` *and* with
+ * `--onto`, we do not want to re-generate the root commits.
+ */
+#define TODO_LIST_ROOT_WITH_ONTO (1U << 6)
+
int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
const char **argv, unsigned flags);
diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
index 7a37235768..39e348de16 100755
--- a/t/t3427-rebase-subtree.sh
+++ b/t/t3427-rebase-subtree.sh
@@ -93,4 +93,15 @@ test_expect_success 'Rebase -Xsubtree --keep-empty --onto
commit' '
verbose test "$(commit_message HEAD)" = "Empty commit"
'
+test_expect_success 'Rebase -Xsubtree --keep-empty --rebase-merges --onto
commit' '
+ reset_rebase &&
+ git checkout -b rebase-merges-onto to-rebase &&
+ test_must_fail git rebase -Xsubtree=files_subtree --keep-empty
--rebase-merges --onto files-master --root &&
+ : first pick results in no changes &&
+ git rebase --continue &&
+ verbose test "$(commit_message HEAD~2)" = "master4" &&
+ verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
+ verbose test "$(commit_message HEAD)" = "Empty commit"
+'
+
test_done
--
gitgitgadget
[PATCH 11/12] t3418: test `rebase -r` with merge strategies
From: Johannes Schindelin There is a test case in this script that verifies that `git rebase --preserve-merges` works all right with non-default merge strategies or non-default merge strategy options. Now that `git rebase --rebase-merges` learned about merge strategies, let's copy-edit this test case to verify that that works as intended, too. Signed-off-by: Johannes Schindelin --- t/t3418-rebase-continue.sh | 14 ++ 1 file changed, 14 insertions(+) diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index bdaa511bb0..fbf9addfd1 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -120,6 +120,20 @@ test_expect_success REBASE_P 'rebase passes merge strategy options correctly' ' git rebase --continue ' +test_expect_success 'rebase -r passes merge strategy options correctly' ' + rm -fr .git/rebase-* && + git reset --hard commit-new-file-F3-on-topic-branch && + test_commit merge-theirs && + git reset --hard HEAD^ && + test_commit some-other-commit && + test_tick && + git merge --no-ff merge-theirs && + FAKE_LINES="1 3 edit 4 5 7 8 9" git rebase -i -f -r -m \ + -s recursive --strategy-option=theirs HEAD~2 && + test_commit force-change-ours && + git rebase --continue +' + test_expect_success '--skip after failed fixup cleans commit message' ' test_when_finished "test_might_fail git rebase --abort" && git checkout -b with-conflicting-fixup && -- gitgitgadget
[PATCH 00/12] rebase -r: support merge strategies other than recursive
This is the most notable shortcoming that --rebase-merges has, still, relative to --preserve-merges' capabilities: it does not support passing custom merge strategies or custom merge strategy options. Let's fix this. While working on this patch series, of course I tried to copy-edit the test cases we have, to cover --preserve-merges' support for merge strategies. Oh my, did I regret this decision as soon as my eyes set sight on t3427-rebase-subtree.sh! At first I tried my best to make heads or tails of t3427, for way too long. In the end the only way to understand what the heck it tries to do was to actually fix it. That's why this patch series looks as if it focuses on t3427 rather than on adding support for custom merge strategies to the --rebase-merges mode. As a consolation to myself, this work was actually worth it, surprising as that may look. Not only is t3427 now really easy to understand, adding that test case for --rebase-merges -Xsubtree tickled the sequencer enough to reveal a long-standing bug: the --onto option was simply ignored when passed together with --rebase-merges and --root. For good measure, this patch series addresses this bug, too. Johannes Schindelin (12): t3427: add a clarifying comment t3427: simplify the `setup` test case significantly t3427: move the `filter-branch` invocation into the `setup` case t3427: condense the unnecessarily repetitive test cases into three t3427: fix erroneous assumption t3427: accommodate for the `rebase --merge` backend having been replaced t3427: fix another incorrect assumption t3427: mark two test cases as requiring support for `git rebase -p` rebase -r: support merge strategies other than `recursive` t/lib-rebase: prepare for testing `git rebase --rebase-merges` t3418: test `rebase -r` with merge strategies rebase -r: do not (re-)generate root commits with `--root` *and* `--onto` Documentation/git-rebase.txt | 2 - builtin/rebase.c | 16 +-- sequencer.c| 18 ++- sequencer.h| 6 + t/lib-rebase.sh| 8 +- t/t3418-rebase-continue.sh | 14 +++ t/t3422-rebase-incompatible-options.sh | 10 -- t/t3427-rebase-subtree.sh | 150 - t/t3430-rebase-merges.sh | 21 9 files changed, 134 insertions(+), 111 deletions(-) base-commit: 082ef75b7bfc90ac236afbb857a9552a026832b8 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-294%2Fdscho%2Frebase-r-with-strategies-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-294/dscho/rebase-r-with-strategies-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/294 -- gitgitgadget
[PATCH 3/3] rebase docs: recommend `-r` over `-p`
From: Johannes Schindelin The `--preserve-merges` option is now deprecated in favor of `--rebase-merges`; Let's stop recommending the former. Signed-off-by: Johannes Schindelin --- Documentation/git-rebase.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index f5e6ae3907..5e4e927647 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -675,7 +675,8 @@ $ git rebase -i HEAD~5 And move the first patch to the end of the list. -You might want to preserve merges, if you have a history like this: +You might want to recreate merge commits, e.g. if you have a history +like this: -- X @@ -689,7 +690,7 @@ Suppose you want to rebase the side branch starting at "A" to "Q". Make sure that the current HEAD is "B", and call - -$ git rebase -i -p --onto Q O +$ git rebase -i -r --onto Q O - Reordering and editing commits usually creates untested intermediate -- gitgitgadget
Re: [PATCH v2] rebase -r: always reword merge -c
Johannes Schindelin writes: > I think this one fell through the cracks (at least I failed to find it in > `pu`), Thanks, I think I was waiting for an Ack or two, but then the thread was buried.
Re: [PATCH v2] rebase -r: always reword merge -c
Hi Junio,
I think this one fell through the cracks (at least I failed to find it in
`pu`), but I deem it a bug fix worthy of including in v2.22.0.
Ciao,
Dscho
On Thu, 2 May 2019, Phillip Wood wrote:
> From: Phillip Wood
>
> If a merge can be fast-forwarded then make sure that we still edit the
> commit message if the user specifies -c. The implementation follows the
> same pattern that is used for ordinary rewords that are fast-forwarded.
>
> Signed-off-by: Phillip Wood
> ---
> Thanks to Dscho for his comments on v1, I've changed the test as he suggested.
>
> Range-diff:
> 1: 0532b70644 ! 1: 738799241a rebase -r: always reword merge -c
> @@ -40,9 +40,12 @@
>
> +test_expect_success 'fast-forward merge -c still rewords' '
> + git checkout -b fast-forward-merge-c H &&
> -+ set_fake_editor &&
> -+ FAKE_COMMIT_MESSAGE=edited GIT_SEQUENCE_EDITOR="echo merge -c H G >" \
> -+ git rebase -ir @^ &&
> ++ (
> ++ set_fake_editor &&
> ++ FAKE_COMMIT_MESSAGE=edited \
> ++ GIT_SEQUENCE_EDITOR="echo merge -c H G >" \
> ++ git rebase -ir @^
> ++ ) &&
> + echo edited >expected &&
> + git log --pretty=format:%B -1 >actual &&
> + test_cmp expected actual
>
> sequencer.c | 5 +
> t/t3430-rebase-merges.sh | 13 +
> 2 files changed, 18 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index 0db410d590..ff8565e7a8 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3248,6 +3248,10 @@ static int do_merge(struct repository *r,
> rollback_lock_file(&lock);
> ret = fast_forward_to(r, &commit->object.oid,
> &head_commit->object.oid, 0, opts);
> + if (flags & TODO_EDIT_MERGE_MSG) {
> + run_commit_flags |= AMEND_MSG;
> + goto fast_forward_edit;
> + }
> goto leave_merge;
> }
>
> @@ -3351,6 +3355,7 @@ static int do_merge(struct repository *r,
>* value (a negative one would indicate that the `merge`
>* command needs to be rescheduled).
>*/
> + fast_forward_edit:
> ret = !!run_git_commit(r, git_path_merge_msg(r), opts,
> run_commit_flags);
>
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index 4c69255ee6..01238d4b6e 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -164,6 +164,19 @@ test_expect_success 'failed `merge ` does not
> crash' '
> grep "^Merge branch ${SQ}G${SQ}$" .git/rebase-merge/message
> '
>
> +test_expect_success 'fast-forward merge -c still rewords' '
> + git checkout -b fast-forward-merge-c H &&
> + (
> + set_fake_editor &&
> + FAKE_COMMIT_MESSAGE=edited \
> + GIT_SEQUENCE_EDITOR="echo merge -c H G >" \
> + git rebase -ir @^
> + ) &&
> + echo edited >expected &&
> + git log --pretty=format:%B -1 >actual &&
> + test_cmp expected actual
> +'
> +
> test_expect_success 'with a branch tip that was cherry-picked already' '
> git checkout -b already-upstream master &&
> base="$(git rev-parse --verify HEAD)" &&
> --
> 2.21.0
>
>
>
Re: [PATCH v2] rebase -r: always reword merge -c
Hi Phillip, On Thu, 2 May 2019, Phillip Wood wrote: > From: Phillip Wood > > If a merge can be fast-forwarded then make sure that we still edit the > commit message if the user specifies -c. The implementation follows the > same pattern that is used for ordinary rewords that are fast-forwarded. > > Signed-off-by: Phillip Wood > --- ACK! Thank you, Dscho
[PATCH v2] rebase -r: always reword merge -c
From: Phillip Wood
If a merge can be fast-forwarded then make sure that we still edit the
commit message if the user specifies -c. The implementation follows the
same pattern that is used for ordinary rewords that are fast-forwarded.
Signed-off-by: Phillip Wood
---
Thanks to Dscho for his comments on v1, I've changed the test as he suggested.
Range-diff:
1: 0532b70644 ! 1: 738799241a rebase -r: always reword merge -c
@@ -40,9 +40,12 @@
+test_expect_success 'fast-forward merge -c still rewords' '
+ git checkout -b fast-forward-merge-c H &&
-+ set_fake_editor &&
-+ FAKE_COMMIT_MESSAGE=edited GIT_SEQUENCE_EDITOR="echo merge -c H G >" \
-+ git rebase -ir @^ &&
++ (
++ set_fake_editor &&
++ FAKE_COMMIT_MESSAGE=edited \
++ GIT_SEQUENCE_EDITOR="echo merge -c H G >" \
++ git rebase -ir @^
++ ) &&
+ echo edited >expected &&
+ git log --pretty=format:%B -1 >actual &&
+ test_cmp expected actual
sequencer.c | 5 +
t/t3430-rebase-merges.sh | 13 +
2 files changed, 18 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 0db410d590..ff8565e7a8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3248,6 +3248,10 @@ static int do_merge(struct repository *r,
rollback_lock_file(&lock);
ret = fast_forward_to(r, &commit->object.oid,
&head_commit->object.oid, 0, opts);
+ if (flags & TODO_EDIT_MERGE_MSG) {
+ run_commit_flags |= AMEND_MSG;
+ goto fast_forward_edit;
+ }
goto leave_merge;
}
@@ -3351,6 +3355,7 @@ static int do_merge(struct repository *r,
* value (a negative one would indicate that the `merge`
* command needs to be rescheduled).
*/
+ fast_forward_edit:
ret = !!run_git_commit(r, git_path_merge_msg(r), opts,
run_commit_flags);
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 4c69255ee6..01238d4b6e 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -164,6 +164,19 @@ test_expect_success 'failed `merge ` does not
crash' '
grep "^Merge branch ${SQ}G${SQ}$" .git/rebase-merge/message
'
+test_expect_success 'fast-forward merge -c still rewords' '
+ git checkout -b fast-forward-merge-c H &&
+ (
+ set_fake_editor &&
+ FAKE_COMMIT_MESSAGE=edited \
+ GIT_SEQUENCE_EDITOR="echo merge -c H G >" \
+ git rebase -ir @^
+ ) &&
+ echo edited >expected &&
+ git log --pretty=format:%B -1 >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'with a branch tip that was cherry-picked already' '
git checkout -b already-upstream master &&
base="$(git rev-parse --verify HEAD)" &&
--
2.21.0
Re: [PATCH] rebase -r: always reword merge -c
Hi Phillip,
On Tue, 30 Apr 2019, Phillip Wood wrote:
> On 29/04/2019 17:14, Johannes Schindelin wrote:
> > Hi Phillip,
> >
> > On Fri, 26 Apr 2019, Phillip Wood wrote:
> >
> > > ret = !!run_git_commit(r, git_path_merge_msg(r), opts,
> > > run_commit_flags);
> > >
> > > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> > > index 4c69255ee6..3d484a3c72 100755
> > > --- a/t/t3430-rebase-merges.sh
> > > +++ b/t/t3430-rebase-merges.sh
> > > @@ -164,6 +164,16 @@ test_expect_success 'failed `merge ` does not
> > > crash' '
> > > grep "^Merge branch ${SQ}G${SQ}$" .git/rebase-merge/message
> > > '
> > >
> > > +test_expect_success 'fast-forward merge -c still rewords' '
> > > + git checkout -b fast-forward-merge-c H &&
> > > + set_fake_editor &&
> >
> > set_fake_editor affects global state AFAIR (setting and exporting
> > `EDITOR`), therefore this would need to be run in a subshell, i.e.
> > enclosed in parentheses.
>
> The other test files are not very consistent about that. I'll re-roll. Note
> that I do not export any FAKE_* variables, so later tests should not be
> affected even if the fake editor runs.
AFAIR I tried my best to avoid `set_fake_editor` altogether and instead
preferred `write_script`/`test_config core.editor` combos in t3430.
Ciao,
Dscho
Re: [PATCH] rebase -r: always reword merge -c
Hi Dscho
On 29/04/2019 17:14, Johannes Schindelin wrote:
Hi Phillip,
On Fri, 26 Apr 2019, Phillip Wood wrote:
From: Phillip Wood
If a merge can be fast-forwarded then make sure that we still edit the
commit message if the user specifies -c. The implementation follows the
same pattern that is used for ordinary rewords that are fast-forwarded.
Signed-off-by: Phillip Wood
---
OMG I was bitten twice by this very bug in the past week, and planned on
looking into it next week. Thanks for beating me to it.
Two comments:
diff --git a/sequencer.c b/sequencer.c
index 0db410d590..ff8565e7a8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3248,6 +3248,10 @@ static int do_merge(struct repository *r,
rollback_lock_file(&lock);
ret = fast_forward_to(r, &commit->object.oid,
&head_commit->object.oid, 0, opts);
+ if (flags & TODO_EDIT_MERGE_MSG) {
+ run_commit_flags |= AMEND_MSG;
+ goto fast_forward_edit;
+ }
goto leave_merge;
}
@@ -3351,6 +3355,7 @@ static int do_merge(struct repository *r,
* value (a negative one would indicate that the `merge`
* command needs to be rescheduled).
*/
+ fast_forward_edit:
It is *slightly* awkward that this is an `else` arm of an `if (ret)`, but
I do not necessarily think that it would be better to move the label
before the `if` than what you did; Your version comes out more readable,
still.
I did wonder about adding braces but I'm not sure that makes it any
clearer. I agree having the label before the `if (ret)` would be less
clear as the reader has to then think what ret will be in that case to
work out what will happen.
ret = !!run_git_commit(r, git_path_merge_msg(r), opts,
run_commit_flags);
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 4c69255ee6..3d484a3c72 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -164,6 +164,16 @@ test_expect_success 'failed `merge ` does not
crash' '
grep "^Merge branch ${SQ}G${SQ}$" .git/rebase-merge/message
'
+test_expect_success 'fast-forward merge -c still rewords' '
+ git checkout -b fast-forward-merge-c H &&
+ set_fake_editor &&
set_fake_editor affects global state AFAIR (setting and exporting
`EDITOR`), therefore this would need to be run in a subshell, i.e.
enclosed in parentheses.
The other test files are not very consistent about that. I'll re-roll.
Note that I do not export any FAKE_* variables, so later tests should
not be affected even if the fake editor runs.
Best Wishes
Phillip
+ FAKE_COMMIT_MESSAGE=edited GIT_SEQUENCE_EDITOR="echo merge -c H G >" \
+ git rebase -ir @^ &&
+ echo edited >expected &&
+ git log --pretty=format:%B -1 >actual &&
+ test_cmp expected actual
+'
+
The rest looks good, thank you!
Dscho
test_expect_success 'with a branch tip that was cherry-picked already' '
git checkout -b already-upstream master &&
base="$(git rev-parse --verify HEAD)" &&
--
2.21.0
Re: [PATCH] rebase -r: always reword merge -c
Hi Phillip,
On Fri, 26 Apr 2019, Phillip Wood wrote:
> From: Phillip Wood
>
> If a merge can be fast-forwarded then make sure that we still edit the
> commit message if the user specifies -c. The implementation follows the
> same pattern that is used for ordinary rewords that are fast-forwarded.
>
> Signed-off-by: Phillip Wood
> ---
OMG I was bitten twice by this very bug in the past week, and planned on
looking into it next week. Thanks for beating me to it.
Two comments:
> diff --git a/sequencer.c b/sequencer.c
> index 0db410d590..ff8565e7a8 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3248,6 +3248,10 @@ static int do_merge(struct repository *r,
> rollback_lock_file(&lock);
> ret = fast_forward_to(r, &commit->object.oid,
> &head_commit->object.oid, 0, opts);
> + if (flags & TODO_EDIT_MERGE_MSG) {
> + run_commit_flags |= AMEND_MSG;
> + goto fast_forward_edit;
> + }
> goto leave_merge;
> }
>
> @@ -3351,6 +3355,7 @@ static int do_merge(struct repository *r,
>* value (a negative one would indicate that the `merge`
>* command needs to be rescheduled).
>*/
> + fast_forward_edit:
It is *slightly* awkward that this is an `else` arm of an `if (ret)`, but
I do not necessarily think that it would be better to move the label
before the `if` than what you did; Your version comes out more readable,
still.
> ret = !!run_git_commit(r, git_path_merge_msg(r), opts,
> run_commit_flags);
>
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index 4c69255ee6..3d484a3c72 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -164,6 +164,16 @@ test_expect_success 'failed `merge ` does not
> crash' '
> grep "^Merge branch ${SQ}G${SQ}$" .git/rebase-merge/message
> '
>
> +test_expect_success 'fast-forward merge -c still rewords' '
> + git checkout -b fast-forward-merge-c H &&
> + set_fake_editor &&
set_fake_editor affects global state AFAIR (setting and exporting
`EDITOR`), therefore this would need to be run in a subshell, i.e.
enclosed in parentheses.
> + FAKE_COMMIT_MESSAGE=edited GIT_SEQUENCE_EDITOR="echo merge -c H G >" \
> + git rebase -ir @^ &&
> + echo edited >expected &&
> + git log --pretty=format:%B -1 >actual &&
> + test_cmp expected actual
> +'
> +
The rest looks good, thank you!
Dscho
> test_expect_success 'with a branch tip that was cherry-picked already' '
> git checkout -b already-upstream master &&
> base="$(git rev-parse --verify HEAD)" &&
> --
> 2.21.0
>
>
[PATCH] rebase -r: always reword merge -c
From: Phillip Wood
If a merge can be fast-forwarded then make sure that we still edit the
commit message if the user specifies -c. The implementation follows the
same pattern that is used for ordinary rewords that are fast-forwarded.
Signed-off-by: Phillip Wood
---
sequencer.c | 5 +
t/t3430-rebase-merges.sh | 10 ++
2 files changed, 15 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 0db410d590..ff8565e7a8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3248,6 +3248,10 @@ static int do_merge(struct repository *r,
rollback_lock_file(&lock);
ret = fast_forward_to(r, &commit->object.oid,
&head_commit->object.oid, 0, opts);
+ if (flags & TODO_EDIT_MERGE_MSG) {
+ run_commit_flags |= AMEND_MSG;
+ goto fast_forward_edit;
+ }
goto leave_merge;
}
@@ -3351,6 +3355,7 @@ static int do_merge(struct repository *r,
* value (a negative one would indicate that the `merge`
* command needs to be rescheduled).
*/
+ fast_forward_edit:
ret = !!run_git_commit(r, git_path_merge_msg(r), opts,
run_commit_flags);
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 4c69255ee6..3d484a3c72 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -164,6 +164,16 @@ test_expect_success 'failed `merge ` does not
crash' '
grep "^Merge branch ${SQ}G${SQ}$" .git/rebase-merge/message
'
+test_expect_success 'fast-forward merge -c still rewords' '
+ git checkout -b fast-forward-merge-c H &&
+ set_fake_editor &&
+ FAKE_COMMIT_MESSAGE=edited GIT_SEQUENCE_EDITOR="echo merge -c H G >" \
+ git rebase -ir @^ &&
+ echo edited >expected &&
+ git log --pretty=format:%B -1 >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'with a branch tip that was cherry-picked already' '
git checkout -b already-upstream master &&
base="$(git rev-parse --verify HEAD)" &&
--
2.21.0
[PATCH 0/1] mingw: fix uname -r test
In df5218b4c30b (config.mak.uname: support MSys2, 2016-01-13), I obviously made the assumption that calling uname -r in MSYS2 would always yield a version number starting with "2". That is incorrect, though, as uname -r reports the version of the Cygwin runtime on which the current MSYS2 runtime is based. This sadly breaks our build as soon as we upgrade to an MSYS2 runtime that is based on Cygwin v3.0.2. Happily, this patch fixes that. Johannes Schindelin (1): mingw: allow building with an MSYS2 runtime v3.x config.mak.uname | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 6e0cc6776106079ed4efa0cc9abace4107657abf Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-160%2Fdscho%2Fmsys2-3.x-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-160/dscho/msys2-3.x-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/160 -- gitgitgadget
[PATCH 09/20] diff-parseopt: convert -R
Signed-off-by: Nguyễn Thái Ngọc Duy
---
diff.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/diff.c b/diff.c
index 51d22f63fa..689dc11684 100644
--- a/diff.c
+++ b/diff.c
@@ -5216,6 +5216,8 @@ static void prep_parse_options(struct diff_options
*options)
diff_opt_relative),
OPT_BOOL('a', "text", &options->flags.text,
N_("treat all files as text")),
+ OPT_BOOL('R', NULL, &options->flags.reverse_diff,
+N_("swap two inputs, reverse the diff")),
{ OPTION_CALLBACK, 0, "output", options, N_(""),
N_("Output to a specific file"),
PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5248,9 +5250,7 @@ int diff_opt_parse(struct diff_options *options,
return ac;
/* flags options */
- if (!strcmp(arg, "-R"))
- options->flags.reverse_diff = 1;
- else if (!strcmp(arg, "--follow"))
+ if (!strcmp(arg, "--follow"))
options->flags.follow_renames = 1;
else if (!strcmp(arg, "--no-follow")) {
options->flags.follow_renames = 0;
--
2.21.0.rc1.337.gdf7f8d0522
[PATCH 45/76] diff.c: convert -R
Signed-off-by: Nguyễn Thái Ngọc Duy
---
diff.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/diff.c b/diff.c
index d342d06399..13de6ea35c 100644
--- a/diff.c
+++ b/diff.c
@@ -5220,6 +5220,8 @@ static void prep_parse_options(struct diff_options
*options)
diff_opt_relative),
OPT_BOOL('a', "text", &options->flags.text,
N_("treat all files as text")),
+ OPT_BOOL('R', NULL, &options->flags.reverse_diff,
+N_("swap two inputs, reverse the diff")),
{ OPTION_CALLBACK, 0, "output", options, N_(""),
N_("Output to a specific file"),
PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5252,9 +5254,7 @@ int diff_opt_parse(struct diff_options *options,
return ac;
/* flags options */
- if (!strcmp(arg, "-R"))
- options->flags.reverse_diff = 1;
- else if (!strcmp(arg, "--follow"))
+ if (!strcmp(arg, "--follow"))
options->flags.follow_renames = 1;
else if (!strcmp(arg, "--no-follow")) {
options->flags.follow_renames = 0;
--
2.20.0.482.g66447595a7
Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
Johannes Schindelin writes: > You misunderstand. In this case it is crucial to read the regression test > first. The fix does not make much sense before one understands the > condition under which the order of the code statements matters. I am not sure what you mean. It sounds as if you want to use diff-orderfile to present change for t/ before changes to other files are presented in format-patch output to help readers, and I think that may make sense for certain cases. It may even include this case. But that is not incompatible with "avoid showing the patch that updates the code to fix breakages separately, ending up with showing the changes to t/ that are mostly about s/_failure/_success/ and readers are forced to go back to the previous patch to remind themselves what the broken scenario was about; by keeping it in a single patch, the readers can get the tests in one piece".
Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
Hi Junio, On Tue, 13 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> For a trivially small change/fix like this, it is OK and even > >> preferrable to make 1+2 a single step, as applying t/ part only to > >> try to see the breakage (or "am"ing everything and then "diff | > >> apply -R" the part outside t/ for the same purpose) is easy enough. > > > > I disagree. It helps both development and porting to different branches to > > be able to cherry-pick the regression test individually. Please do not ask > > me to violate this hard-learned principle. > > A trivially small change/fix like this, by definition (of "trivial" > and "small" ness), it is trivial to develop and port to different > branches a single patch, and test with just one half (either the > test part or the code-change part) of the change reversed, to ensure > that the codebase is broken without the code-change and to > demonstrate that the code-change does fix the problem revealed by > the test change. And "porting" by cherry-picking a single patch is > always easier than two patch series. > > So you may disagree all you want in your project, but do not make > reviewer's lives unnecessarily harder in this project. You misunderstand. In this case it is crucial to read the regression test first. The fix does not make much sense before one understands the condition under which the order of the code statements matters. By trying to force me to smoosh them together, you are trying to force me to combine them in one patch where you would read the (now seemingly non-sensical) fix first, and only then the test. That's just really unhelpful. If I were a reviewer, I would want it presented in the way it *was* presented. I firmly believe most reviewers would. Ciao, Dscho
Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
Johannes Schindelin writes: >> For a trivially small change/fix like this, it is OK and even >> preferrable to make 1+2 a single step, as applying t/ part only to >> try to see the breakage (or "am"ing everything and then "diff | >> apply -R" the part outside t/ for the same purpose) is easy enough. > > I disagree. It helps both development and porting to different branches to > be able to cherry-pick the regression test individually. Please do not ask > me to violate this hard-learned principle. A trivially small change/fix like this, by definition (of "trivial" and "small" ness), it is trivial to develop and port to different branches a single patch, and test with just one half (either the test part or the code-change part) of the change reversed, to ensure that the codebase is broken without the code-change and to demonstrate that the code-change does fix the problem revealed by the test change. And "porting" by cherry-picking a single patch is always easier than two patch series. So you may disagree all you want in your project, but do not make reviewer's lives unnecessarily harder in this project. Thanks.
Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
Hi Junio, On Tue, 13 Nov 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > From: Johannes Schindelin > > > > When calling `merge` on a branch that has already been merged, that > > `merge` is skipped quietly, but currently a MERGE_HEAD file is being > > left behind and will then be grabbed by the next `pick` (that did > > not want to create a *merge* commit). > > > > Demonstrate this. > > > > Signed-off-by: Johannes Schindelin > > --- > > t/t3430-rebase-merges.sh | 16 > > 1 file changed, 16 insertions(+) > > For a trivially small change/fix like this, it is OK and even > preferrable to make 1+2 a single step, as applying t/ part only to > try to see the breakage (or "am"ing everything and then "diff | > apply -R" the part outside t/ for the same purpose) is easy enough. I disagree. It helps both development and porting to different branches to be able to cherry-pick the regression test individually. Please do not ask me to violate this hard-learned principle. > Because the patch 2 with your method ends up showing only the test > set-up part in the context by changing _failure to _success, without > showing what end-user visible breakage the step fixed, which usually > comes near the end of the added test piece. A single patch that > gives tests that ought to succeed would not force the readers to > switch between patches 1 and 2 while reading the fix. That is why I put in a verbose commit message, so that you do not have to guess. And even the test title talks about this. Seriously, I am very much opposed to changing the patches in the direction you suggested. In my mind, they would make the story substantially worse. Thank you for your review, Dscho > > Of course, the above would not apply for a more involved case where > the actual fix to the code needs to span multiple patches. > > Thanks. > > > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > > index aa7bfc88ec..1f08a33687 100755 > > --- a/t/t3430-rebase-merges.sh > > +++ b/t/t3430-rebase-merges.sh > > @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' ' > > grep "G: +G" actual > > ' > > > > +test_expect_failure '--continue after resolving conflicts after a merge' ' > > + git checkout -b already-has-g E && > > + git cherry-pick E..G && > > + test_commit H2 && > > + > > + git checkout -b conflicts-in-merge H && > > + test_commit H2 H2.t conflicts H2-conflict && > > + test_must_fail git rebase -r already-has-g && > > + grep conflicts H2.t && > > Is this making sure that the above test_must_fail succeeded because > of a conflict and not due to any other failure? I would have used > "ls-files -u H2.t" to see if the index is unmerged, which probably > is a more direct way to test what this is trying to test, but if we > are in the conflicted state, the one side of << == >> has this > string (the other has "H2" in it, presumably?), so in practice this > should be good enough. > > > + echo resolved >H2.t && > > + git add -u && > > and we resolve to continue. > > > + git rebase --continue && > > + test_must_fail git rev-parse --verify HEAD^2 && > > Even if we made an octopus by mistake, the above will catch it, > which is good. > > > + test_path_is_missing .git/MERGE_HEAD > > +' > > + > > test_done > > And from the proposed log message, I am reading that the last two > things (i.e. resulting tip is a child with a single parent and there > is no leftover MERGE_HEAD file) fail without the fix. > > This is enough material to convince me or anybody that the bug is > worth fixing. Thanks for being careful noticing a glitch during > your real (and otherwise unrelated to the bug) work and following > through. >
Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
"Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > When calling `merge` on a branch that has already been merged, that > `merge` is skipped quietly, but currently a MERGE_HEAD file is being > left behind and will then be grabbed by the next `pick` (that did > not want to create a *merge* commit). > > Demonstrate this. > > Signed-off-by: Johannes Schindelin > --- > t/t3430-rebase-merges.sh | 16 > 1 file changed, 16 insertions(+) For a trivially small change/fix like this, it is OK and even preferrable to make 1+2 a single step, as applying t/ part only to try to see the breakage (or "am"ing everything and then "diff | apply -R" the part outside t/ for the same purpose) is easy enough. Because the patch 2 with your method ends up showing only the test set-up part in the context by changing _failure to _success, without showing what end-user visible breakage the step fixed, which usually comes near the end of the added test piece. A single patch that gives tests that ought to succeed would not force the readers to switch between patches 1 and 2 while reading the fix. Of course, the above would not apply for a more involved case where the actual fix to the code needs to span multiple patches. Thanks. > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > index aa7bfc88ec..1f08a33687 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' ' > grep "G: +G" actual > ' > > +test_expect_failure '--continue after resolving conflicts after a merge' ' > + git checkout -b already-has-g E && > + git cherry-pick E..G && > + test_commit H2 && > + > + git checkout -b conflicts-in-merge H && > + test_commit H2 H2.t conflicts H2-conflict && > + test_must_fail git rebase -r already-has-g && > + grep conflicts H2.t && Is this making sure that the above test_must_fail succeeded because of a conflict and not due to any other failure? I would have used "ls-files -u H2.t" to see if the index is unmerged, which probably is a more direct way to test what this is trying to test, but if we are in the conflicted state, the one side of << == >> has this string (the other has "H2" in it, presumably?), so in practice this should be good enough. > + echo resolved >H2.t && > + git add -u && and we resolve to continue. > + git rebase --continue && > + test_must_fail git rev-parse --verify HEAD^2 && Even if we made an octopus by mistake, the above will catch it, which is good. > + test_path_is_missing .git/MERGE_HEAD > +' > + > test_done And from the proposed log message, I am reading that the last two things (i.e. resulting tip is a child with a single parent and there is no leftover MERGE_HEAD file) fail without the fix. This is enough material to convince me or anybody that the bug is worth fixing. Thanks for being careful noticing a glitch during your real (and otherwise unrelated to the bug) work and following through.
[PATCH 2/5] rebase -r: do not write MERGE_HEAD unless needed
From: Johannes Schindelin
When we detect that a `merge` can be skipped because the merged commit
is already an ancestor of HEAD, we do not need to commit, therefore
writing the MERGE_HEAD file is useless.
It is actually worse than useless: a subsequent `git commit` will pick
it up and think that we want to merge that commit, still.
To avoid that, move the code that writes the MERGE_HEAD file to a
location where we already know that the `merge` cannot be skipped.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 8
t/t3430-rebase-merges.sh | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 9e1ab3a2a7..7a9cd81afb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3191,10 +3191,6 @@ static int do_merge(struct commit *commit, const char
*arg, int arg_len,
}
merge_commit = to_merge->item;
- write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
- git_path_merge_head(the_repository), 0);
- write_message("no-ff", 5, git_path_merge_mode(the_repository), 0);
-
bases = get_merge_bases(head_commit, merge_commit);
if (bases && oideq(&merge_commit->object.oid,
&bases->item->object.oid)) {
@@ -3203,6 +3199,10 @@ static int do_merge(struct commit *commit, const char
*arg, int arg_len,
goto leave_merge;
}
+ write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
+ git_path_merge_head(the_repository), 0);
+ write_message("no-ff", 5, git_path_merge_mode(the_repository), 0);
+
for (j = bases; j; j = j->next)
commit_list_insert(j->item, &reversed);
free_commit_list(bases);
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 1f08a33687..cc5646836f 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -396,7 +396,7 @@ test_expect_success 'with --autosquash and --exec' '
grep "G: +G" actual
'
-test_expect_failure '--continue after resolving conflicts after a merge' '
+test_expect_success '--continue after resolving conflicts after a merge' '
git checkout -b already-has-g E &&
git cherry-pick E..G &&
test_commit H2 &&
--
gitgitgadget
[PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
From: Johannes Schindelin When calling `merge` on a branch that has already been merged, that `merge` is skipped quietly, but currently a MERGE_HEAD file is being left behind and will then be grabbed by the next `pick` (that did not want to create a *merge* commit). Demonstrate this. Signed-off-by: Johannes Schindelin --- t/t3430-rebase-merges.sh | 16 1 file changed, 16 insertions(+) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index aa7bfc88ec..1f08a33687 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' ' grep "G: +G" actual ' +test_expect_failure '--continue after resolving conflicts after a merge' ' + git checkout -b already-has-g E && + git cherry-pick E..G && + test_commit H2 && + + git checkout -b conflicts-in-merge H && + test_commit H2 H2.t conflicts H2-conflict && + test_must_fail git rebase -r already-has-g && + grep conflicts H2.t && + echo resolved >H2.t && + git add -u && + git rebase --continue && + test_must_fail git rev-parse --verify HEAD^2 && + test_path_is_missing .git/MERGE_HEAD +' + test_done -- gitgitgadget
wishlist: git grep -r
I often use "grep -r $pattern" to recursively grep a source tree. If that takes too long, I hit ^C and tag "git" in front of the command line and re-run it. git then complains "error: unknown switch `r'" because "git grep" is naturally recursive. Could we have "git grep -r" accept the argument for compatibility? Other important grep switches like "-i" are compatible, adding -r would improve usability. Thanks, Christoph signature.asc Description: PGP signature
[PATCH v5 05/23] blame.c: rename "repo" argument to "r"
The current naming convention for 'struct repository *' is 'r' for
temporary variables or arguments. I did not notice this. Since we're
updating blame.c again in the next patch, let's fix this.
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: Junio C Hamano
---
blame.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/blame.c b/blame.c
index aca06f4b12..98bf50d89a 100644
--- a/blame.c
+++ b/blame.c
@@ -90,7 +90,7 @@ static struct blame_origin *get_origin(struct commit *commit,
const char *path)
-static void verify_working_tree_path(struct repository *repo,
+static void verify_working_tree_path(struct repository *r,
struct commit *work_tree, const char *path)
{
struct commit_list *parents;
@@ -102,15 +102,15 @@ static void verify_working_tree_path(struct repository
*repo,
unsigned mode;
if (!get_tree_entry(commit_oid, path, &blob_oid, &mode) &&
- oid_object_info(repo, &blob_oid, NULL) == OBJ_BLOB)
+ oid_object_info(r, &blob_oid, NULL) == OBJ_BLOB)
return;
}
- pos = index_name_pos(repo->index, path, strlen(path));
+ pos = index_name_pos(r->index, path, strlen(path));
if (pos >= 0)
; /* path is in the index */
- else if (-1 - pos < repo->index->cache_nr &&
- !strcmp(repo->index->cache[-1 - pos]->name, path))
+ else if (-1 - pos < r->index->cache_nr &&
+!strcmp(r->index->cache[-1 - pos]->name, path))
; /* path is in the index, unmerged */
else
die("no such path '%s' in HEAD", path);
@@ -166,7 +166,7 @@ static void set_commit_buffer_from_strbuf(struct commit *c,
struct strbuf *sb)
* Prepare a dummy commit that represents the work tree (or staged) item.
* Note that annotating work tree item never works in the reverse.
*/
-static struct commit *fake_working_tree_commit(struct repository *repo,
+static struct commit *fake_working_tree_commit(struct repository *r,
struct diff_options *opt,
const char *path,
const char *contents_from)
@@ -183,7 +183,7 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
unsigned mode;
struct strbuf msg = STRBUF_INIT;
- read_index(repo->index);
+ read_index(r->index);
time(&now);
commit = alloc_commit_node(the_repository);
commit->object.parsed = 1;
@@ -195,7 +195,7 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
parent_tail = append_parent(parent_tail, &head_oid);
append_merge_parents(parent_tail);
- verify_working_tree_path(repo, commit, path);
+ verify_working_tree_path(r, commit, path);
origin = make_origin(commit, path);
@@ -253,7 +253,7 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
if (strbuf_read(&buf, 0, 0) < 0)
die_errno("failed to read from stdin");
}
- convert_to_git(repo->index, path, buf.buf, buf.len, &buf, 0);
+ convert_to_git(r->index, path, buf.buf, buf.len, &buf, 0);
origin->file.ptr = buf.buf;
origin->file.size = buf.len;
pretend_object_file(buf.buf, buf.len, OBJ_BLOB, &origin->blob_oid);
@@ -264,28 +264,28 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
* bits; we are not going to write this index out -- we just
* want to run "diff-index --cached".
*/
- discard_index(repo->index);
- read_index(repo->index);
+ discard_index(r->index);
+ read_index(r->index);
len = strlen(path);
if (!mode) {
- int pos = index_name_pos(repo->index, path, len);
+ int pos = index_name_pos(r->index, path, len);
if (0 <= pos)
- mode = repo->index->cache[pos]->ce_mode;
+ mode = r->index->cache[pos]->ce_mode;
else
/* Let's not bother reading from HEAD tree */
mode = S_IFREG | 0644;
}
- ce = make_empty_cache_entry(repo->index, len);
+ ce = make_empty_cache_entry(r->index, len);
oidcpy(&ce->oid, &origin->blob_oid);
memcpy(ce->name, path, len);
ce->ce_flags = create_ce_flags(0);
ce->ce_namelen = len;
ce->ce_mode = create_ce_mode(mode);
- add_index_entry(repo->index,
Re: [PATCH v4 05/23] blame.c: rename "repo" argument to "r"
Nguyễn Thái Ngọc Duy writes: > The current naming convention for 'struct repository *' is 'r' for > temporary variables or arguments. I did not notice this. Since we're > updating blame.c again in the next patch, let's fix this. It is likely that we end up having to refer to an in-core repository object in many places, so giving a short-and-sweet 'r' to it makes quite a lot of sense. One thing we may want to do as preparation related to this effort is to sweep the codebase to make sure we do not use 'r' as a variable that refers to anything other than an in-core repository object. Thanks.
[PATCH v4 05/23] blame.c: rename "repo" argument to "r"
The current naming convention for 'struct repository *' is 'r' for
temporary variables or arguments. I did not notice this. Since we're
updating blame.c again in the next patch, let's fix this.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
blame.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/blame.c b/blame.c
index aca06f4b12..98bf50d89a 100644
--- a/blame.c
+++ b/blame.c
@@ -90,7 +90,7 @@ static struct blame_origin *get_origin(struct commit *commit,
const char *path)
-static void verify_working_tree_path(struct repository *repo,
+static void verify_working_tree_path(struct repository *r,
struct commit *work_tree, const char *path)
{
struct commit_list *parents;
@@ -102,15 +102,15 @@ static void verify_working_tree_path(struct repository
*repo,
unsigned mode;
if (!get_tree_entry(commit_oid, path, &blob_oid, &mode) &&
- oid_object_info(repo, &blob_oid, NULL) == OBJ_BLOB)
+ oid_object_info(r, &blob_oid, NULL) == OBJ_BLOB)
return;
}
- pos = index_name_pos(repo->index, path, strlen(path));
+ pos = index_name_pos(r->index, path, strlen(path));
if (pos >= 0)
; /* path is in the index */
- else if (-1 - pos < repo->index->cache_nr &&
- !strcmp(repo->index->cache[-1 - pos]->name, path))
+ else if (-1 - pos < r->index->cache_nr &&
+!strcmp(r->index->cache[-1 - pos]->name, path))
; /* path is in the index, unmerged */
else
die("no such path '%s' in HEAD", path);
@@ -166,7 +166,7 @@ static void set_commit_buffer_from_strbuf(struct commit *c,
struct strbuf *sb)
* Prepare a dummy commit that represents the work tree (or staged) item.
* Note that annotating work tree item never works in the reverse.
*/
-static struct commit *fake_working_tree_commit(struct repository *repo,
+static struct commit *fake_working_tree_commit(struct repository *r,
struct diff_options *opt,
const char *path,
const char *contents_from)
@@ -183,7 +183,7 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
unsigned mode;
struct strbuf msg = STRBUF_INIT;
- read_index(repo->index);
+ read_index(r->index);
time(&now);
commit = alloc_commit_node(the_repository);
commit->object.parsed = 1;
@@ -195,7 +195,7 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
parent_tail = append_parent(parent_tail, &head_oid);
append_merge_parents(parent_tail);
- verify_working_tree_path(repo, commit, path);
+ verify_working_tree_path(r, commit, path);
origin = make_origin(commit, path);
@@ -253,7 +253,7 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
if (strbuf_read(&buf, 0, 0) < 0)
die_errno("failed to read from stdin");
}
- convert_to_git(repo->index, path, buf.buf, buf.len, &buf, 0);
+ convert_to_git(r->index, path, buf.buf, buf.len, &buf, 0);
origin->file.ptr = buf.buf;
origin->file.size = buf.len;
pretend_object_file(buf.buf, buf.len, OBJ_BLOB, &origin->blob_oid);
@@ -264,28 +264,28 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
* bits; we are not going to write this index out -- we just
* want to run "diff-index --cached".
*/
- discard_index(repo->index);
- read_index(repo->index);
+ discard_index(r->index);
+ read_index(r->index);
len = strlen(path);
if (!mode) {
- int pos = index_name_pos(repo->index, path, len);
+ int pos = index_name_pos(r->index, path, len);
if (0 <= pos)
- mode = repo->index->cache[pos]->ce_mode;
+ mode = r->index->cache[pos]->ce_mode;
else
/* Let's not bother reading from HEAD tree */
mode = S_IFREG | 0644;
}
- ce = make_empty_cache_entry(repo->index, len);
+ ce = make_empty_cache_entry(r->index, len);
oidcpy(&ce->oid, &origin->blob_oid);
memcpy(ce->name, path, len);
ce->ce_flags = create_ce_flags(0);
ce->ce_namelen = len;
ce->ce_mode = create_ce_mode(mode);
- add_index_entry(repo->index, ce,
+ add_ind
[PATCH v3 05/23] blame.c: rename "repo" argument to "r"
The current naming convention for 'struct repository *' is 'r' for
temporary variables or arguments. I did not notice this. Since we're
updating blame.c again in the next patch, let's fix this.
---
blame.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/blame.c b/blame.c
index aca06f4b12..98bf50d89a 100644
--- a/blame.c
+++ b/blame.c
@@ -90,7 +90,7 @@ static struct blame_origin *get_origin(struct commit *commit,
const char *path)
-static void verify_working_tree_path(struct repository *repo,
+static void verify_working_tree_path(struct repository *r,
struct commit *work_tree, const char *path)
{
struct commit_list *parents;
@@ -102,15 +102,15 @@ static void verify_working_tree_path(struct repository
*repo,
unsigned mode;
if (!get_tree_entry(commit_oid, path, &blob_oid, &mode) &&
- oid_object_info(repo, &blob_oid, NULL) == OBJ_BLOB)
+ oid_object_info(r, &blob_oid, NULL) == OBJ_BLOB)
return;
}
- pos = index_name_pos(repo->index, path, strlen(path));
+ pos = index_name_pos(r->index, path, strlen(path));
if (pos >= 0)
; /* path is in the index */
- else if (-1 - pos < repo->index->cache_nr &&
- !strcmp(repo->index->cache[-1 - pos]->name, path))
+ else if (-1 - pos < r->index->cache_nr &&
+!strcmp(r->index->cache[-1 - pos]->name, path))
; /* path is in the index, unmerged */
else
die("no such path '%s' in HEAD", path);
@@ -166,7 +166,7 @@ static void set_commit_buffer_from_strbuf(struct commit *c,
struct strbuf *sb)
* Prepare a dummy commit that represents the work tree (or staged) item.
* Note that annotating work tree item never works in the reverse.
*/
-static struct commit *fake_working_tree_commit(struct repository *repo,
+static struct commit *fake_working_tree_commit(struct repository *r,
struct diff_options *opt,
const char *path,
const char *contents_from)
@@ -183,7 +183,7 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
unsigned mode;
struct strbuf msg = STRBUF_INIT;
- read_index(repo->index);
+ read_index(r->index);
time(&now);
commit = alloc_commit_node(the_repository);
commit->object.parsed = 1;
@@ -195,7 +195,7 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
parent_tail = append_parent(parent_tail, &head_oid);
append_merge_parents(parent_tail);
- verify_working_tree_path(repo, commit, path);
+ verify_working_tree_path(r, commit, path);
origin = make_origin(commit, path);
@@ -253,7 +253,7 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
if (strbuf_read(&buf, 0, 0) < 0)
die_errno("failed to read from stdin");
}
- convert_to_git(repo->index, path, buf.buf, buf.len, &buf, 0);
+ convert_to_git(r->index, path, buf.buf, buf.len, &buf, 0);
origin->file.ptr = buf.buf;
origin->file.size = buf.len;
pretend_object_file(buf.buf, buf.len, OBJ_BLOB, &origin->blob_oid);
@@ -264,28 +264,28 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
* bits; we are not going to write this index out -- we just
* want to run "diff-index --cached".
*/
- discard_index(repo->index);
- read_index(repo->index);
+ discard_index(r->index);
+ read_index(r->index);
len = strlen(path);
if (!mode) {
- int pos = index_name_pos(repo->index, path, len);
+ int pos = index_name_pos(r->index, path, len);
if (0 <= pos)
- mode = repo->index->cache[pos]->ce_mode;
+ mode = r->index->cache[pos]->ce_mode;
else
/* Let's not bother reading from HEAD tree */
mode = S_IFREG | 0644;
}
- ce = make_empty_cache_entry(repo->index, len);
+ ce = make_empty_cache_entry(r->index, len);
oidcpy(&ce->oid, &origin->blob_oid);
memcpy(ce->name, path, len);
ce->ce_flags = create_ce_flags(0);
ce->ce_namelen = len;
ce->ce_mode = create_ce_mode(mode);
- add_index_entry(repo->index, ce,
+ add_index_entry(r->index, ce,
[PATCH v2 05/24] blame.c: rename "repo" argument to "r"
The current naming convention for 'struct repository *' is 'r' for
temporary variables or arguments. I did not notice this. Since we're
updating blame.c again in the next patch, let's fix this.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
blame.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/blame.c b/blame.c
index aca06f4b12..98bf50d89a 100644
--- a/blame.c
+++ b/blame.c
@@ -90,7 +90,7 @@ static struct blame_origin *get_origin(struct commit *commit,
const char *path)
-static void verify_working_tree_path(struct repository *repo,
+static void verify_working_tree_path(struct repository *r,
struct commit *work_tree, const char *path)
{
struct commit_list *parents;
@@ -102,15 +102,15 @@ static void verify_working_tree_path(struct repository
*repo,
unsigned mode;
if (!get_tree_entry(commit_oid, path, &blob_oid, &mode) &&
- oid_object_info(repo, &blob_oid, NULL) == OBJ_BLOB)
+ oid_object_info(r, &blob_oid, NULL) == OBJ_BLOB)
return;
}
- pos = index_name_pos(repo->index, path, strlen(path));
+ pos = index_name_pos(r->index, path, strlen(path));
if (pos >= 0)
; /* path is in the index */
- else if (-1 - pos < repo->index->cache_nr &&
- !strcmp(repo->index->cache[-1 - pos]->name, path))
+ else if (-1 - pos < r->index->cache_nr &&
+!strcmp(r->index->cache[-1 - pos]->name, path))
; /* path is in the index, unmerged */
else
die("no such path '%s' in HEAD", path);
@@ -166,7 +166,7 @@ static void set_commit_buffer_from_strbuf(struct commit *c,
struct strbuf *sb)
* Prepare a dummy commit that represents the work tree (or staged) item.
* Note that annotating work tree item never works in the reverse.
*/
-static struct commit *fake_working_tree_commit(struct repository *repo,
+static struct commit *fake_working_tree_commit(struct repository *r,
struct diff_options *opt,
const char *path,
const char *contents_from)
@@ -183,7 +183,7 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
unsigned mode;
struct strbuf msg = STRBUF_INIT;
- read_index(repo->index);
+ read_index(r->index);
time(&now);
commit = alloc_commit_node(the_repository);
commit->object.parsed = 1;
@@ -195,7 +195,7 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
parent_tail = append_parent(parent_tail, &head_oid);
append_merge_parents(parent_tail);
- verify_working_tree_path(repo, commit, path);
+ verify_working_tree_path(r, commit, path);
origin = make_origin(commit, path);
@@ -253,7 +253,7 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
if (strbuf_read(&buf, 0, 0) < 0)
die_errno("failed to read from stdin");
}
- convert_to_git(repo->index, path, buf.buf, buf.len, &buf, 0);
+ convert_to_git(r->index, path, buf.buf, buf.len, &buf, 0);
origin->file.ptr = buf.buf;
origin->file.size = buf.len;
pretend_object_file(buf.buf, buf.len, OBJ_BLOB, &origin->blob_oid);
@@ -264,28 +264,28 @@ static struct commit *fake_working_tree_commit(struct
repository *repo,
* bits; we are not going to write this index out -- we just
* want to run "diff-index --cached".
*/
- discard_index(repo->index);
- read_index(repo->index);
+ discard_index(r->index);
+ read_index(r->index);
len = strlen(path);
if (!mode) {
- int pos = index_name_pos(repo->index, path, len);
+ int pos = index_name_pos(r->index, path, len);
if (0 <= pos)
- mode = repo->index->cache[pos]->ce_mode;
+ mode = r->index->cache[pos]->ce_mode;
else
/* Let's not bother reading from HEAD tree */
mode = S_IFREG | 0644;
}
- ce = make_empty_cache_entry(repo->index, len);
+ ce = make_empty_cache_entry(r->index, len);
oidcpy(&ce->oid, &origin->blob_oid);
memcpy(ce->name, path, len);
ce->ce_flags = create_ce_flags(0);
ce->ce_namelen = len;
ce->ce_mode = create_ce_mode(mode);
- add_index_entry(repo->index, ce,
+ add_ind
[PATCH v3 1/2] t3430: demonstrate what -r, --autosquash & --exec should do
From: Johannes Schindelin The --exec option's implementation is not really well-prepared for --rebase-merges. Demonstrate this. Signed-off-by: Johannes Schindelin --- t/t3430-rebase-merges.sh | 17 + 1 file changed, 17 insertions(+) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 9e6229727..0bf5eaa37 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -363,4 +363,21 @@ test_expect_success 'octopus merges' ' EOF ' +test_expect_failure 'with --autosquash and --exec' ' + git checkout -b with-exec H && + echo Booh >B.t && + test_tick && + git commit --fixup B B.t && + write_script show.sh <<-\EOF && + subject="$(git show -s --format=%s HEAD)" + content="$(git diff HEAD^! | tail -n 1)" + echo "$subject: $content" + EOF + test_tick && + git rebase -ir --autosquash --exec ./show.sh A >actual && + grep "B: +Booh" actual && + grep "E: +Booh" actual && + grep "G: +G" actual +' + test_done -- gitgitgadget
[PATCH v2 1/2] t3430: demonstrate what -r, --autosquash & --exec should do
From: Johannes Schindelin The --exec option's implementation is not really well-prepared for --rebase-merges. Demonstrate this. Signed-off-by: Johannes Schindelin --- t/t3430-rebase-merges.sh | 17 + 1 file changed, 17 insertions(+) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 9e6229727..0bf5eaa37 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -363,4 +363,21 @@ test_expect_success 'octopus merges' ' EOF ' +test_expect_failure 'with --autosquash and --exec' ' + git checkout -b with-exec H && + echo Booh >B.t && + test_tick && + git commit --fixup B B.t && + write_script show.sh <<-\EOF && + subject="$(git show -s --format=%s HEAD)" + content="$(git diff HEAD^! | tail -n 1)" + echo "$subject: $content" + EOF + test_tick && + git rebase -ir --autosquash --exec ./show.sh A >actual && + grep "B: +Booh" actual && + grep "E: +Booh" actual && + grep "G: +G" actual +' + test_done -- gitgitgadget
[PATCH 1/2] t3430: demonstrate what -r, --autosquash & --exec should do
From: Johannes Schindelin The --exec option's implementation is not really well-prepared for --rebase-merges. Demonstrate this. Signed-off-by: Johannes Schindelin --- t/t3430-rebase-merges.sh | 17 + 1 file changed, 17 insertions(+) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 9e6229727..0bf5eaa37 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -363,4 +363,21 @@ test_expect_success 'octopus merges' ' EOF ' +test_expect_failure 'with --autosquash and --exec' ' + git checkout -b with-exec H && + echo Booh >B.t && + test_tick && + git commit --fixup B B.t && + write_script show.sh <<-\EOF && + subject="$(git show -s --format=%s HEAD)" + content="$(git diff HEAD^! | tail -n 1)" + echo "$subject: $content" + EOF + test_tick && + git rebase -ir --autosquash --exec ./show.sh A >actual && + grep "B: +Booh" actual && + grep "E: +Booh" actual && + grep "G: +G" actual +' + test_done -- gitgitgadget
[PATCH] TO-SQUASH: replace the_repository with arbitrary r
Signed-off-by: Derrick Stolee
---
This should just be squashed into PATCH 2.
builtin/replace.c | 5 ++---
replace-object.c | 2 +-
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index 5f34659071..d0b1cdb061 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -57,9 +57,8 @@ static int show_reference(struct repository *r, const char
*refname,
if (get_oid(refname, &object))
return error("Failed to resolve '%s' as a valid
ref.", refname);
- obj_type = oid_object_info(the_repository, &object,
- NULL);
- repl_type = oid_object_info(the_repository, oid, NULL);
+ obj_type = oid_object_info(r, &object, NULL);
+ repl_type = oid_object_info(r, oid, NULL);
printf("%s (%s) -> %s (%s)\n", refname,
type_name(obj_type),
oid_to_hex(oid), type_name(repl_type));
diff --git a/replace-object.c b/replace-object.c
index 01a5a59a35..017f02f8ef 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -26,7 +26,7 @@ static int register_replace_ref(struct repository *r,
oidcpy(&repl_obj->replacement, oid);
/* Register new object */
- if (oidmap_put(the_repository->objects->replace_map, repl_obj))
+ if (oidmap_put(r->objects->replace_map, repl_obj))
die("duplicate replace ref: %s", refname);
return 0;
--
2.18.0.118.gd4f65b8d14
Re: [PATCH 0/3] rebase -r: support octopus merges
Johannes Sixt writes: > Am 12.07.2018 um 18:26 schrieb Junio C Hamano: >> Johannes Schindelin writes: >>> A much more meaningful measure would be: how many octopus merge commits >>> have been pushed to GitHub in the past two weeks. I don't think I have the >>> technical means to answer that question, though. >> >> It does not mean that misusing a feature is a good thing and should >> be encouraged if many misguided people do so. > > Just recently I had to rebuild the version of git-gui that comes with > Git 2.18.0 before it was released: > > https://github.com/j6t/git-gui-ng/commit/f07ae1d7f07b036d78a3d4706e6cb4102e623fb3 > > I think that an octopus merge is the right tool for the task. Am I > misguided? It could be used and it is the right tool are two different things, I would think.
Re: [PATCH 0/3] rebase -r: support octopus merges
Am 12.07.2018 um 18:26 schrieb Junio C Hamano: Johannes Schindelin writes: A much more meaningful measure would be: how many octopus merge commits have been pushed to GitHub in the past two weeks. I don't think I have the technical means to answer that question, though. It does not mean that misusing a feature is a good thing and should be encouraged if many misguided people do so. Just recently I had to rebuild the version of git-gui that comes with Git 2.18.0 before it was released: https://github.com/j6t/git-gui-ng/commit/f07ae1d7f07b036d78a3d4706e6cb4102e623fb3 I think that an octopus merge is the right tool for the task. Am I misguided? -- Hannes
Re: [PATCH 0/3] rebase -r: support octopus merges
Johannes Schindelin writes: > Git is used in *so many* different scenarios, and both in terms of > commits/day as well as overall repository size *and* development speed, That misses another important factor, though. How well the project uses the tool, iow, how canonical should its way of using the tool be considered and encouraged to be imitated by others. > A much more meaningful measure would be: how many octopus merge commits > have been pushed to GitHub in the past two weeks. I don't think I have the > technical means to answer that question, though. It does not mean that misusing a feature is a good thing and should be encouraged if many misguided people do so.
Re: [PATCH 0/3] rebase -r: support octopus merges
Hi Stefan, On Wed, 11 Jul 2018, Stefan Beller wrote: > On Wed, Jul 11, 2018 at 10:35 AM Junio C Hamano wrote: > > To be honest, I am not sure if there still are people who use > > octopus > > The latest merge with more than 2 parents in linux.git is df958569dbaa > (Merge branches 'acpi-tables' and 'acpica', 2018-07-05), although > looking through the log of octopusses I get the impression that mostly > Rafael J. Wysocki is really keen on these. > :-) IMO core Git contributors seriously need to forget about using the Linux kernel repository as the gold standard when looking how Git is used. Git is used in *so many* different scenarios, and both in terms of commits/day as well as overall repository size *and* development speed, Linux is not even in the "smack down the middle" category. Compared to what is being done with Git on a daily basis, the Linux kernel repository (and project structure) is relatively small. A much more meaningful measure would be: how many octopus merge commits have been pushed to GitHub in the past two weeks. I don't think I have the technical means to answer that question, though. In any case, the Git project is run in such a way that even having a feature used even by just single user whose name happens to be Andrew Morton declares that feature off-limits for deprecation. When applying this context to `--rebase-merges` and Octopus merges, even a single user would be sufficient for us to support that feature. And I am sure that there are more than just a dozen users of this feature. Ciao, Dscho
Re: [PATCH 0/3] rebase -r: support octopus merges
Hi Junio, On Wed, 11 Jul 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > The `--rebase-merges` option of `git rebase` generates todo lists that > > include the merge commits that are to be rebased. > > > > To keep things simpler to review, I decided to support only regular, > > 2-parent merge commits first. > > > > With this patch series, support is extended to cover also octopus > > merges. > > ;-) > > To be honest, I am not sure if there still are people who use octopus > (even though I freely admit that I am guilty of inventing the term and > the mechanism), given its downsides, primary one of which is that it > makes bisection less efficient. > > Nevertheless, this *is* the right thing to do from feature completeness > point of view. Thanks for following it through. --preserve-merges supports octopus merges, IIRC. And as I want to deprecate --preserve-merges, --rebase-merges *has* to support octopus merges, whether you or I would ever use them or not. Ciao, Dscho
Re: [PATCH 0/3] rebase -r: support octopus merges
On Wed, Jul 11, 2018 at 10:35 AM Junio C Hamano wrote: > To be honest, I am not sure if there still are people who use > octopus The latest merge with more than 2 parents in linux.git is df958569dbaa (Merge branches 'acpi-tables' and 'acpica', 2018-07-05), although looking through the log of octopusses I get the impression that mostly Rafael J. Wysocki is really keen on these. :-)
Re: [PATCH 0/3] rebase -r: support octopus merges
"Johannes Schindelin via GitGitGadget" writes: > The `--rebase-merges` option of `git rebase` generates todo lists that > include the merge commits that are to be rebased. > > To keep things simpler to review, I decided to support only regular, 2-parent > merge commits first. > > With this patch series, support is extended to cover also octopus merges. ;-) To be honest, I am not sure if there still are people who use octopus (even though I freely admit that I am guilty of inventing the term and the mechanism), given its downsides, primary one of which is that it makes bisection less efficient. Nevertheless, this *is* the right thing to do from feature completeness point of view. Thanks for following it through. > > Johannes Schindelin (3): > merge: allow reading the merge commit message from a file > rebase --rebase-merges: add support for octopus merges > rebase --rebase-merges: adjust man page for octopus support > > Documentation/git-merge.txt | 10 ++- > Documentation/git-rebase.txt | 7 +- > builtin/merge.c | 32 +++ > sequencer.c | 168 ++- > t/t3430-rebase-merges.sh | 34 +++ > 5 files changed, 204 insertions(+), 47 deletions(-) > > > base-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9 > Published-As: > https://github.com/gitgitgadget/git/releases/tags/pr-8%2Fdscho%2Fsequencer-and-octopus-merges-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git > pr-8/dscho/sequencer-and-octopus-merges-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/8
[PATCH 0/3] rebase -r: support octopus merges
The `--rebase-merges` option of `git rebase` generates todo lists that include the merge commits that are to be rebased. To keep things simpler to review, I decided to support only regular, 2-parent merge commits first. With this patch series, support is extended to cover also octopus merges. Johannes Schindelin (3): merge: allow reading the merge commit message from a file rebase --rebase-merges: add support for octopus merges rebase --rebase-merges: adjust man page for octopus support Documentation/git-merge.txt | 10 ++- Documentation/git-rebase.txt | 7 +- builtin/merge.c | 32 +++ sequencer.c | 168 ++- t/t3430-rebase-merges.sh | 34 +++ 5 files changed, 204 insertions(+), 47 deletions(-) base-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-8%2Fdscho%2Fsequencer-and-octopus-merges-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-8/dscho/sequencer-and-octopus-merges-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/8 -- gitgitgadget
Re: "git rm" seems to do recursive removal even without "-r"
On Tue, 10 Oct 2017, Heiko Voigt wrote: > On Sun, Oct 08, 2017 at 07:56:20AM -0400, Robert P. J. Day wrote: > > but as i asked in my earlier post, if i wanted to remove *all* files > > with names of "Makefile*", why can't i use: > > > > $ git rm 'Makefile*' > > > > just as i used: > > > > $ git rm '*.c' > > > > are those not both acceptable fileglobs? why does the former > > clearly only match the top-level Makefile, and refuse to cross > > directory boundaries? > > Maybe think about it this way: The only difference between git's > globbing and the default shell globbing is that the '/' in a path > has a special meaning. The shells expansion stops at a '/' but git > does not. > > So with *.c the shell matches: blabla.c, blub.c, ... but not > subdir/bla.c, subdir/blub.c, ... since it only considers files in > the current directory. A little different for Makefile* that will > also match Makefile.bla, Makefile/bla or Makefile_bla/blub in shell > but not subdir/Makefile or bla.Makefile. Basically anything directly > in *this* directory that *starts* with 'Makefile'. > > Git on the other hand does not consider '/' to be special. So *.c > matches all of the path above: bla.c, blub.c, subdir/bla.c, > subdir/blub.c. Basically any file below the current directory with a > path that ends in '.c'. With Makefile* it is the opposite: Every > file below the current directory that *starts* with 'Makefile'. So > Makefile.bla, Makefile/bla, ... but also not subdir/Makefile or > bla.Makefile. ok, i believe i finally appreciate what is happening here, and perhaps my first contribution will be a minor addition to the "git-rm" man page to introduce a couple examples explaining these intricacies, since they're not immediately obvious. i'll put something together and submit it to the list. thank you all for your patience in explaining this. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git rm" seems to do recursive removal even without "-r"
On Tue, 10 Oct 2017, Paul Smith wrote: > On Tue, 2017-10-10 at 04:36 -0400, Robert P. J. Day wrote: > > ah, now *that* is a compelling rationale that justifies the > > underlying weirdness. but it still doesn't explain the different > > behaviour between: > > > > $ git rm -n 'Makefile*' > > $ git rm -n '*Makefile' > > I explained that behavior in the email up-thread from this reply: yup, sorry i missed it. man, it's been an educational thread. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git rm" seems to do recursive removal even without "-r"
On Tue, 2017-10-10 at 04:36 -0400, Robert P. J. Day wrote: > ah, now *that* is a compelling rationale that justifies the > underlying weirdness. but it still doesn't explain the different > behaviour between: > > $ git rm -n 'Makefile*' > $ git rm -n '*Makefile' I explained that behavior in the email up-thread from this reply: > Globbing in "git rm" matches on the FULL PATH, not just the file name. > So, if you have a list of Makefiles in your repository like: > > Makefile > foo/Makefile > bar/Makefile > > Then 'Makefile*' only matches the first one, since 'Makefile*' doesn't > match 'foo/Makefile' or 'bar/Makefile'. > > If you you worry that '*Makefile' will match things you don't want to > match, you'll have to use: > > git rm -n Makefile '*/Makefile'
Re: "git rm" seems to do recursive removal even without "-r"
On Sun, Oct 08, 2017 at 07:56:20AM -0400, Robert P. J. Day wrote: > but as i asked in my earlier post, if i wanted to remove *all* files > with names of "Makefile*", why can't i use: > > $ git rm 'Makefile*' > > just as i used: > > $ git rm '*.c' > > are those not both acceptable fileglobs? why does the former clearly > only match the top-level Makefile, and refuse to cross directory > boundaries? Maybe think about it this way: The only difference between git's globbing and the default shell globbing is that the '/' in a path has a special meaning. The shells expansion stops at a '/' but git does not. So with *.c the shell matches: blabla.c, blub.c, ... but not subdir/bla.c, subdir/blub.c, ... since it only considers files in the current directory. A little different for Makefile* that will also match Makefile.bla, Makefile/bla or Makefile_bla/blub in shell but not subdir/Makefile or bla.Makefile. Basically anything directly in *this* directory that *starts* with 'Makefile'. Git on the other hand does not consider '/' to be special. So *.c matches all of the path above: bla.c, blub.c, subdir/bla.c, subdir/blub.c. Basically any file below the current directory with a path that ends in '.c'. With Makefile* it is the opposite: Every file below the current directory that *starts* with 'Makefile'. So Makefile.bla, Makefile/bla, ... but also not subdir/Makefile or bla.Makefile. Maybe that helps... Cheers Heiko
Re: "git rm" seems to do recursive removal even without "-r"
"Robert P. J. Day" writes: > underlying weirdness. but it still doesn't explain the different > behaviour between: > > $ git rm -n 'Makefile*' > $ git rm -n '*Makefile' > > in the linux kernel source tree, the first form matches only the > single, top-level Makefile, while the second form gets *all* of them > recursively, even though those globs should be equivalent in terms of > matching all files named "Makefile". > > am i misunderstanding something? We are matching what Git cares/knows about aka "the paths in the index" to pathspec patterns. What are these paths in the index? In Linux kernel sources, there are quite a many but here are examples that are enough to explain the above: Makefile COPYING fs/Makefile fs/ext4/Makefile Which one of these four match patterh "Makefile*", which is "the first letter is 'M', the second letter is 'a', ,, the eighth letter is 'e', and anything else can follow to the end"? Yes, only the first one. Which one of these four match pattern "*Makefile", then? "Anything can appear as leading substring, but then 'M', 'a', 'k', ..., and finally 'e' must appear at the end"? Note that these "start from the paths in the index that match the pathspec patterns" have nothing to do with "recursive". It happens way before we decide to go recursive (or not). We are not going down recursively from the paths in the index that are matched by pathspec patterns with the above two "git rm" requests (because there is no "-r" there), but even if we were, because these three Makefile files are not directories, there is nothing to remove recursively underneath them.
Re: "git rm" seems to do recursive removal even without "-r"
On Mon, 9 Oct 2017, Jeff King wrote: > On Sun, Oct 08, 2017 at 04:42:27PM -0400, Theodore Ts'o wrote: > > > On Sun, Oct 08, 2017 at 03:44:14PM -0400, Robert P. J. Day wrote: > > > > > > > > find | xargs git rm > > > > > > > > myself. > > > > > > that's what i would have normally used until i learned about > > > git's magical globbing capabilities, and i'm going to go back to > > > using it, because git's magical globbing capabilities now scare > > > me. > > > > Hmm, I wonder if the reason why git's magically globbing > > capabilities even exist at all is for those poor benighted souls > > on Windows, for which their shell (and associated utilities) > > doesn't have advanced tools like "find" and "xargs" > > One benefit of globbing with Git is that it restricts the matches > only to tracked files. That matters a lot when you have a very broad > glob (e.g., like you might use with "git grep") because it avoids > looking at cruft like generated files (or even inside .git). ah, now *that* is a compelling rationale that justifies the underlying weirdness. but it still doesn't explain the different behaviour between: $ git rm -n 'Makefile*' $ git rm -n '*Makefile' in the linux kernel source tree, the first form matches only the single, top-level Makefile, while the second form gets *all* of them recursively, even though those globs should be equivalent in terms of matching all files named "Makefile". am i misunderstanding something? rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git rm" seems to do recursive removal even without "-r"
On Sun, Oct 08, 2017 at 04:42:27PM -0400, Theodore Ts'o wrote: > On Sun, Oct 08, 2017 at 03:44:14PM -0400, Robert P. J. Day wrote: > > > > > > find | xargs git rm > > > > > > myself. > > > > that's what i would have normally used until i learned about git's > > magical globbing capabilities, and i'm going to go back to using it, > > because git's magical globbing capabilities now scare me. > > Hmm, I wonder if the reason why git's magically globbing capabilities > even exist at all is for those poor benighted souls on Windows, for > which their shell (and associated utilities) doesn't have advanced > tools like "find" and "xargs" One benefit of globbing with Git is that it restricts the matches only to tracked files. That matters a lot when you have a very broad glob (e.g., like you might use with "git grep") because it avoids looking at cruft like generated files (or even inside .git). -Peff
Re: "git rm" seems to do recursive removal even without "-r"
On Sun, Oct 08, 2017 at 03:44:14PM -0400, Robert P. J. Day wrote: > > > > find | xargs git rm > > > > myself. > > that's what i would have normally used until i learned about git's > magical globbing capabilities, and i'm going to go back to using it, > because git's magical globbing capabilities now scare me. Hmm, I wonder if the reason why git's magically globbing capabilities even exist at all is for those poor benighted souls on Windows, for which their shell (and associated utilities) doesn't have advanced tools like "find" and "xargs" - Ted
Re: "git rm" seems to do recursive removal even without "-r"
On Sun, 8 Oct 2017, Theodore Ts'o wrote: > On Sun, Oct 08, 2017 at 10:32:40AM -0400, Paul Smith wrote: > > Personally I don't use Git's magical globbing capabilities, and > > use "git rm" as if it were UNIX rm. So in your request above I'd > > use: > > > >git rm $(find . -name Makefile) > > > > which I find simpler. > > I have to agree that git's magical globbing capabilities are... > strange. (And apologies to Robert for my earlier post; I didn't > understand what he was complaining about.) I don't use it either, > although I tend to use: > > find | xargs git rm > > myself. that's what i would have normally used until i learned about git's magical globbing capabilities, and i'm going to go back to using it, because git's magical globbing capabilities now scare me. > One thing which is interesting is that not only is the git's magical > globbing capabilities have somewhat unusual semantics, the how > globbing is done in .gitignore entries are completely different. i know ... it would have made way more sense to try to be consistent. oh, well, live and learn. at least now i'm aware of the weirdness. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git rm" seems to do recursive removal even without "-r"
On Sun, Oct 08, 2017 at 10:32:40AM -0400, Paul Smith wrote: > Personally I don't use Git's magical globbing capabilities, and use "git > rm" as if it were UNIX rm. So in your request above I'd use: > >git rm $(find . -name Makefile) > > which I find simpler. I have to agree that git's magical globbing capabilities are... strange. (And apologies to Robert for my earlier post; I didn't understand what he was complaining about.) I don't use it either, although I tend to use: find | xargs git rm myself. One thing which is interesting is that not only is the git's magical globbing capabilities have somewhat unusual semantics, the how globbing is done in .gitignore entries are completely different. Shrug. I put this in the same category as "tabs are significant in Makefile's", "whitespace is significant in python", and "the many varied different behaviours and uses of 'git reset'". They are all idiosyncrancies of semantics of various highly popular tools (which being highly popular, would make changing the details quite difficult due to backwards compatibility concerns, even if we wanted to change them). - Ted
Re: "git rm" seems to do recursive removal even without "-r"
On Sat, 2017-10-07 at 17:55 -0400, Robert P. J. Day wrote: > On Sat, 7 Oct 2017, Paul Smith wrote: > > On Sat, 2017-10-07 at 15:43 -0400, Robert P. J. Day wrote: > > > it's been a long week, so take this in the spirit in which it is > > > intended ... i think the "git rm" command and its man page should be > > > printed out, run through a paper shredder, then set on fire. i can't > > > remember the last time i saw such a thoroughly badly-designed, > > > badly-documented and non-intuitive utility. > > > > "git rm" works the same way that the UNIX rm command has worked, for > > 40+ years now. Very simple, very well designed, and very intuitive > > (IMO). > > > > The major difference is the ability to handle globbing patterns, > > which UNIX rm doesn't do. Maybe the way this is implemented is a > > little confusing, although I just read the man page and it seemed > > pretty clear to me. > > um, wrong. I don't know what part of my comment here you consider "wrong". I've re-read it and I believe everything I said is correct. > > If you don't use glob patterns (or more specifically if you let the > > shell handle glob patterns, which is how I always do it) then there > > is really nothing bizarre about "git rm". Maybe you could be more > > precise in your criticism. > > ok, fine, let me explain why this command is a nightmarish > monstrosity. as i now understand, if i use an escaped wildcard > pattern, "git rm" will *automatically* (with no further guidance from > me, and no warning), operate recursively. so if, in the kernel source > tree, i ran: > > $ git rm \*.c Yes, this is what I said: "IF YOU DON'T USE GLOB PATTERNS (or more specifically if you let the shell handle glob patterns ...) then there is nothing bizarre about "git rm"" (emphasis added). In this example you ARE sending glob patterns to Git, because you're escaping them from the shell. Hence, you might consider the behavior bizarre, at least until you grok it fully. If you want to avoid this, simply use normal shell globbing and don't ask Git to do the globbing for you. Then it behaves exactly as normal UNIX rm, including with the '-r' option, and is very simple. > so if i wanted git to remove, say, all files named "Makefile*" from > everywhere in the linux kernel source tree, the (dry run) command > would be: > > $ git rm -n Makefile\* > > is that it? let's try that: > > $ git rm -n Makefile\* > rm 'Makefile' > $ > > oops. Globbing in "git rm" matches on the FULL PATH, not just the file name. So, if you have a list of Makefiles in your repository like: Makefile foo/Makefile bar/Makefile Then 'Makefile*' only matches the first one, since 'Makefile*' doesn't match 'foo/Makefile' or 'bar/Makefile'. If you you worry that '*Makefile' will match things you don't want to match, you'll have to use: git rm -n Makefile '*/Makefile' Personally I don't use Git's magical globbing capabilities, and use "git rm" as if it were UNIX rm. So in your request above I'd use: git rm $(find . -name Makefile) which I find simpler.
Re: "git rm" seems to do recursive removal even without "-r"
On Sun, 8 Oct 2017, René Scharfe wrote: > [My SMTP server still refuses to accept emails to rpj...@crashcourse.ca > and reports "mailbox unavailable" and "invalid DNS MX or A/ resource > record." So just replying to the list.] > > Am 08.10.2017 um 13:56 schrieb Robert P. J. Day: > >but as i asked in my earlier post, if i wanted to remove *all* files > > with names of "Makefile*", why can't i use: > > > >$ git rm 'Makefile*' > > > > just as i used: > > > >$ git rm '*.c' > > > > are those not both acceptable fileglobs? why does the former clearly > > only match the top-level Makefile, and refuse to cross directory > > boundaries? > > > >$ git rm -n 'Makefile*' > >rm 'Makefile' > >$ > > Try: > > $ git rm -n '*Makefile' > > The whole path is considered. The asterisk there matches any > directory part -- but also any file name prefix. Check the entry > for "pathspec" in gitglossary(7) for more details. but "*Makefile" is not the wildcard pattern i'm interested in. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git rm" seems to do recursive removal even without "-r"
[My SMTP server still refuses to accept emails to rpj...@crashcourse.ca and reports "mailbox unavailable" and "invalid DNS MX or A/ resource record." So just replying to the list.] Am 08.10.2017 um 13:56 schrieb Robert P. J. Day: >but as i asked in my earlier post, if i wanted to remove *all* files > with names of "Makefile*", why can't i use: > >$ git rm 'Makefile*' > > just as i used: > >$ git rm '*.c' > > are those not both acceptable fileglobs? why does the former clearly > only match the top-level Makefile, and refuse to cross directory > boundaries? > >$ git rm -n 'Makefile*' >rm 'Makefile' >$ Try: $ git rm -n '*Makefile' The whole path is considered. The asterisk there matches any directory part -- but also any file name prefix. Check the entry for "pathspec" in gitglossary(7) for more details. René
Re: "git rm" seems to do recursive removal even without "-r"
On 8 October 2017 at 13:56, Robert P. J. Day wrote: > but as i asked in my earlier post, if i wanted to remove *all* files > with names of "Makefile*", why can't i use: > > $ git rm 'Makefile*' > > just as i used: > > $ git rm '*.c' > > are those not both acceptable fileglobs? why does the former clearly > only match the top-level Makefile, and refuse to cross directory > boundaries? > > $ git rm -n 'Makefile*' > rm 'Makefile' > $ Hmmm. The manpage says the following: git rm -f git-*.sh Because this example lets the shell expand the asterisk (i.e. you are listing the files explicitly), it does not remove subdir/git-foo.sh. This implies that `git rm "git-*.sh"` should remove subdir/git-foo.sh. But it doesn't, at least not in my testing. It seems that the globbing only kicks in when the "*" comes first, as you've noted.
Re: "git rm" seems to do recursive removal even without "-r"
On Sun, 8 Oct 2017, Junio C Hamano wrote: > "Robert P. J. Day" writes: > > > ... so if, in the kernel source > > tree, i ran: > > > > $ git rm \*.c > > > > i would end up removing *all* 25,569 "*.c" files in the kernel > > source repository. > > Yes, as that is exactly what the command line asks Git to do. ok, i truly want to understand this, so let me dig through this carefully. i can now see (from the man page and the recent explanations) that "git rm" will accept *escaped* fileglobs to remove and that, further, "File globbing matches across directory boundaries." which is why, in the linux kernel source tree, if i run one of: $ git rm \*.c $ git rm '*.c' the "git rm" command will internally process the fileglob and apply it across directory boundaries. and that's why, when i try a dry run, i can see the effect it would have on the kernel source: $ git rm -n '*.c' | wc -l 25569 $ > If you said > > $ git rm *.c > > then the shell expands the glob and all Git sees is that you want to > remove a.c b.c d.c ...; if you said "git rm -r *.c", unless b.c is > not a directory, these and only these files are removed. right, that's just regular shell fileglob processing, no surprise there. (let's stick to just file removal for now.) > > however, let's say i wanted to remove, recursively, all files with a > > *precise* (non-globbed) name, such as "Makefile". so i, naively, run: > > > > $ git rm Makefile > > > > guess what ... the lack of globbing means i remove only the single > > Makefile at the top of the working directory. > > Again, that is exactly what you asked Git to do. yes, now i get it -- a lack of fileglob arguments disallows traversing directory boundaries, so one gets the "normal" behaviour. > $ git rm $(find . -name Makefile -print) > > would of course one way to remove all Makefiles. If you let POSIX > shell glob, i.e. > > $ git rm */Makefile > > the asterisk would not expand nothing but a single level, so it may > remove fs/Makefile, but not fs/ext4/Makefile (some shells allow > "wildmatch" expansion so "git rm **/Makefile" may catch the latter > with such a shell). sure, all regular shell fileglob processing. > By letting Git see the glob, i.e. > > $ git rm Makefile \*/Makefile > > you would let Git to go over the paths it knows/cares about to find > ones that match the pathspec pattern and remove them (but not > recursively, even if you had a directory whose name is Makefile; for > that, you would use "-r"). right ... i can now see that '*/Makefile' would pick up all Makefiles *below* the current directory, so you need that initial reference to 'Makefile' to catch the top one. this just seems ... awkward. but as i asked in my earlier post, if i wanted to remove *all* files with names of "Makefile*", why can't i use: $ git rm 'Makefile*' just as i used: $ git rm '*.c' are those not both acceptable fileglobs? why does the former clearly only match the top-level Makefile, and refuse to cross directory boundaries? $ git rm -n 'Makefile*' rm 'Makefile' $ rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git rm" seems to do recursive removal even without "-r"
On Sun, Oct 08, 2017 at 05:07:12AM -0400, Robert P. J. Day wrote: > On Sun, 8 Oct 2017, Junio C Hamano wrote: > > > "Robert P. J. Day" writes: > > > > > ... so if, in the kernel source > > > tree, i ran: > > > > > > $ git rm \*.c > > > > > > i would end up removing *all* 25,569 "*.c" files in the kernel source > > > repository. > > > > Yes, as that is exactly what the command line asks Git to do. > > so if i wanted git to remove, say, all files named "Makefile*" from > everywhere in the linux kernel source tree, the (dry run) command > would be: > > $ git rm -n Makefile\* > > is that it? let's try that: > > $ git rm -n Makefile\* > rm 'Makefile' > $ > > oops. > > rday > So your question is not su much about the recursive option (delete mentioned directories, including their contents), but globbing (expanding the * to any files matching the pattern). The explanation of mentions this: Files to remove. Fileglobs (e.g. *.c) can be given to remove all matching files. This indicates that git itself (not your shell alone) does file globbing. I think the confusing part is that most people have no clear idea of the separation between what the shell sees and interprets, and what the program actually gets. When you execute: $ git rm Makefile\* What git actually sees is this: Makefile* The shell intepreted the \* to mean just '*' and not interpret it itself, and provide that to the executed program. Git, in its turn, would start matching any file to that pattern to see which files matches. If you would execute: $ git rm 'Makefile\*' Git would see: Makefile\* Which does the thing you intended. So you have to deal with 2 levels of programs interpreting the arguments, not just one. Whether '*.c' should match just all .c files in the current dir, or all files ending with .c depends on whether the path seperator is matched by * or not and is a separate discussion. GITGLOSSARY(7) under pathspec mentions this: globGit treats the pattern as a shell glob suitable for consumption by fnmatch(3) with the FNM_PATHNAME flag: wildcards in the pattern will not match a / in the pathname. So that seems to indicate '*.c' should only match .c files in the current dir. I'm not sure why that's not the case. I hope this clears up what's happening a bit, and perhaps can lead to improvements to the documentation so that it's not so surprising. Kind regards, Kevin.
Re: "git rm" seems to do recursive removal even without "-r"
On Sun, 8 Oct 2017, Junio C Hamano wrote: > "Robert P. J. Day" writes: > > > ... so if, in the kernel source > > tree, i ran: > > > > $ git rm \*.c > > > > i would end up removing *all* 25,569 "*.c" files in the kernel source > > repository. > > Yes, as that is exactly what the command line asks Git to do. so if i wanted git to remove, say, all files named "Makefile*" from everywhere in the linux kernel source tree, the (dry run) command would be: $ git rm -n Makefile\* is that it? let's try that: $ git rm -n Makefile\* rm 'Makefile' $ oops. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git rm" seems to do recursive removal even without "-r"
"Robert P. J. Day" writes: > ... so if, in the kernel source > tree, i ran: > > $ git rm \*.c > > i would end up removing *all* 25,569 "*.c" files in the kernel source > repository. Yes, as that is exactly what the command line asks Git to do. If you said $ git rm *.c then the shell expands the glob and all Git sees is that you want to remove a.c b.c d.c ...; if you said "git rm -r *.c", unless b.c is not a directory, these and only these files are removed. > however, let's say i wanted to remove, recursively, all files with a > *precise* (non-globbed) name, such as "Makefile". so i, naively, run: > > $ git rm Makefile > > guess what ... the lack of globbing means i remove only the single > Makefile at the top of the working directory. Again, that is exactly what you asked Git to do. $ git rm $(find . -name Makefile -print) would of course one way to remove all Makefiles. If you let POSIX shell glob, i.e. $ git rm */Makefile the asterisk would not expand nothing but a single level, so it may remove fs/Makefile, but not fs/ext4/Makefile (some shells allow "wildmatch" expansion so "git rm **/Makefile" may catch the latter with such a shell). By letting Git see the glob, i.e. $ git rm Makefile \*/Makefile you would let Git to go over the paths it knows/cares about to find ones that match the pathspec pattern and remove them (but not recursively, even if you had a directory whose name is Makefile; for that, you would use "-r"). Earlier some people mentioned "Unix newbie" in the thread, but I do not think this is about Unix. In general, Unix tools do not perform grobbing themselves but expect the user to tell the shell to do so before the tools see the arguments. In that sense, I do think the combination of "-r" and globbing pathspec may produce a result that looks confusing at first glance. "git rm [-r] ..." (1) walks the paths it knows/cares about, rejecting ones that do not match the ; (2) decides to remove the ones that match; and (3) when it is asked to recursively remove, the ones that are directories are removed together with its contents. If it was not asked to go recursive, it refuses to act on directories. where (1) and (2) are not something the tool needs to worry about---what is given from the command line is the only set of paths that the tool is asked to operate on. These two steps are quite unlike regular Unix tools. Once you decide to give the tool the flexibility that come from taking pathspec, however, steps (1) and (2) do have to happen inside the tool. And great power takes some understanding of the tool on the part of the user to exercise. I suspect that the occasion you would need to use "-r" with "git rm" is a lot less frequent than a plain "rm". Of course, there is no confusion if you do not quote wildcards. By quoting wildcards and not letting the shell to expand them, the user tells Git that the fact _it_ sees the asterisk is because the user is doing so on purpose---so that Git would find paths that match the pattern. Hope this clarifies and helps.
Re: "git rm" seems to do recursive removal even without "-r"
On Sat, 2017-10-07 at 15:43 -0400, Robert P. J. Day wrote: > it's been a long week, so take this in the spirit in which it is > intended ... i think the "git rm" command and its man page should be > printed out, run through a paper shredder, then set on fire. i can't > remember the last time i saw such a thoroughly badly-designed, > badly-documented and non-intuitive utility. "git rm" works the same way that the UNIX rm command has worked, for 40+ years now. Very simple, very well designed, and very intuitive (IMO). The major difference is the ability to handle globbing patterns, which UNIX rm doesn't do. Maybe the way this is implemented is a little confusing, although I just read the man page and it seemed pretty clear to me. If you don't use glob patterns (or more specifically if you let the shell handle glob patterns, which is how I always do it) then there is really nothing bizarre about "git rm". Maybe you could be more precise in your criticism.
Re: "git rm" seems to do recursive removal even without "-r"
On Sat, 7 Oct 2017, Paul Smith wrote: > On Sat, 2017-10-07 at 15:43 -0400, Robert P. J. Day wrote: > > it's been a long week, so take this in the spirit in which it is > > intended ... i think the "git rm" command and its man page should be > > printed out, run through a paper shredder, then set on fire. i can't > > remember the last time i saw such a thoroughly badly-designed, > > badly-documented and non-intuitive utility. > > "git rm" works the same way that the UNIX rm command has worked, for > 40+ years now. Very simple, very well designed, and very intuitive > (IMO). > > The major difference is the ability to handle globbing patterns, > which UNIX rm doesn't do. Maybe the way this is implemented is a > little confusing, although I just read the man page and it seemed > pretty clear to me. um, wrong. > If you don't use glob patterns (or more specifically if you let the > shell handle glob patterns, which is how I always do it) then there > is really nothing bizarre about "git rm". Maybe you could be more > precise in your criticism. ok, fine, let me explain why this command is a nightmarish monstrosity. as i now understand, if i use an escaped wildcard pattern, "git rm" will *automatically* (with no further guidance from me, and no warning), operate recursively. so if, in the kernel source tree, i ran: $ git rm \*.c i would end up removing *all* 25,569 "*.c" files in the kernel source repository. however, let's say i wanted to remove, recursively, all files with a *precise* (non-globbed) name, such as "Makefile". so i, naively, run: $ git rm Makefile guess what ... the lack of globbing means i remove only the single Makefile at the top of the working directory. if that isn't an example of ridiculous, non-intuitive behaviour, i don't know what is. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git rm" seems to do recursive removal even without "-r"
On Sat, 7 Oct 2017, Theodore Ts'o wrote: > On Sat, Oct 07, 2017 at 03:43:43PM -0400, Robert P. J. Day wrote: > > > -r > > > Recursively remove the contents of any directories that match > > > ``. > > > > > > or something. > > > > it's been a long week, so take this in the spirit in which it is > > intended ... i think the "git rm" command and its man page should be > > printed out, run through a paper shredder, then set on fire. i can't > > remember the last time i saw such a thoroughly badly-designed, > > badly-documented and non-intuitive utility. > > > > i'm going to go watch football now and try to forget this horror. > > It sounds like the real issue here is that you are interpreting > "recursively" to mean "globbing". Your original complaint seemed to > be a surprise that "git rm book/\*.asc" would delete all of the files > in the directory "book" that ended in .asc, even without the -r flag. > > That's because the operation of matching *.asc is considered > "globbing". Now if there were directories whose name ended in .asc, > then they would only be deleted if the -r flag is given. Deleting > directories and their contents is what is considered "recursive > removal". > > That's not particularly surprising to me as a long-time Unix/Linux > user/developer, since that's how things work in Unix/Linux: > > % touch 1.d 2.d ; mkdir 3.d 4.d > % /bin/ls > 1.d 2.d 3.d 4.d > % rm -r *.d > % touch 1.d 2.d ; mkdir 3.d 4.d > % rm *.d > rm: cannot remove '3.d': Is a directory > rm: cannot remove '4.d': Is a directory > > I'm going to guess that you don't come from a Unix background? yeah, that must be it, i'm a newbie at linux. let's go with that. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git rm" seems to do recursive removal even without "-r"
On Sat, Oct 07, 2017 at 03:43:43PM -0400, Robert P. J. Day wrote: > > -r > > Recursively remove the contents of any directories that match > > ``. > > > > or something. > > it's been a long week, so take this in the spirit in which it is > intended ... i think the "git rm" command and its man page should be > printed out, run through a paper shredder, then set on fire. i can't > remember the last time i saw such a thoroughly badly-designed, > badly-documented and non-intuitive utility. > > i'm going to go watch football now and try to forget this horror. It sounds like the real issue here is that you are interpreting "recursively" to mean "globbing". Your original complaint seemed to be a surprise that "git rm book/\*.asc" would delete all of the files in the directory "book" that ended in .asc, even without the -r flag. That's because the operation of matching *.asc is considered "globbing". Now if there were directories whose name ended in .asc, then they would only be deleted if the -r flag is given. Deleting directories and their contents is what is considered "recursive removal". That's not particularly surprising to me as a long-time Unix/Linux user/developer, since that's how things work in Unix/Linux: % touch 1.d 2.d ; mkdir 3.d 4.d % /bin/ls 1.d 2.d 3.d 4.d % rm -r *.d % touch 1.d 2.d ; mkdir 3.d 4.d % rm *.d rm: cannot remove '3.d': Is a directory rm: cannot remove '4.d': Is a directory I'm going to guess that you don't come from a Unix background? - Ted
Re: "git rm" seems to do recursive removal even without "-r"
On Sat, 7 Oct 2017, Jeff King wrote: > On Sat, Oct 07, 2017 at 03:32:24PM -0400, Robert P. J. Day wrote: > > > > Nothing, because there is nothing to recurse in the pathspecs you've > > > given. > > > > > > Try: > > > > > > $ git rm drivers > > > fatal: not removing 'drivers' recursively without -r > > > > > > versus > > > > > > $ git rm -r drivers > > > [...removes everything under drivers/...] > > > > that is not what the man page is saying ... it refers to a "leading" > > directory name, not simply a directory name. if it should say simply > > "when a directory name is given", then it should be changed to say > > that. > > It's the leading directory of the files that will be removed. > > An earlier part of the manpage (under ) also says: > > A leading directory name (e.g. dir to remove dir/file1 and dir/file2) > can be given to remove all files in the directory, and recursively all > sub-directories, but this requires the -r option to be explicitly > given. > > which perhaps makes it more clear. Later in "-r", we say: > >-r >Allow recursive removal when a leading directory name is given. > > which I guess is the part you're reading. I think it would be equally > correct to say "leading directory" or just "directory" there. > > Though really, you could give many such directory names, or even match > them with a glob. So a more accurate description might be something > like: > > -r > Recursively remove the contents of any directories that match > ``. > > or something. it's been a long week, so take this in the spirit in which it is intended ... i think the "git rm" command and its man page should be printed out, run through a paper shredder, then set on fire. i can't remember the last time i saw such a thoroughly badly-designed, badly-documented and non-intuitive utility. i'm going to go watch football now and try to forget this horror. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git rm" seems to do recursive removal even without "-r"
On Sat, Oct 07, 2017 at 03:32:24PM -0400, Robert P. J. Day wrote: > > Nothing, because there is nothing to recurse in the pathspecs you've > > given. > > > > Try: > > > > $ git rm drivers > > fatal: not removing 'drivers' recursively without -r > > > > versus > > > > $ git rm -r drivers > > [...removes everything under drivers/...] > > that is not what the man page is saying ... it refers to a "leading" > directory name, not simply a directory name. if it should say simply > "when a directory name is given", then it should be changed to say > that. It's the leading directory of the files that will be removed. An earlier part of the manpage (under ) also says: A leading directory name (e.g. dir to remove dir/file1 and dir/file2) can be given to remove all files in the directory, and recursively all sub-directories, but this requires the -r option to be explicitly given. which perhaps makes it more clear. Later in "-r", we say: -r Allow recursive removal when a leading directory name is given. which I guess is the part you're reading. I think it would be equally correct to say "leading directory" or just "directory" there. Though really, you could give many such directory names, or even match them with a glob. So a more accurate description might be something like: -r Recursively remove the contents of any directories that match ``. or something. -Peff
Re: "git rm" seems to do recursive removal even without "-r"
On Sat, 7 Oct 2017, Jeff King wrote: > On Sat, Oct 07, 2017 at 03:12:01PM -0400, Robert P. J. Day wrote: > > > ok, in that case, can you give me an example where "-r" makes a > > difference, given that the man page refers to "a leading directory > > name being given"? let's use as an example the linux kernel source, > > where there are a *ton* of files named "Makefile" under the drivers/ > > directory. > > > > should there be a difference between: > > > > $ git rm drivers/Makefile > > $ git rm -r drivers/Makefile > > > > clearly, i have a "leading directory name" in both examples above ... > > what should happen differently? > > Nothing, because there is nothing to recurse in the pathspecs you've > given. > > Try: > > $ git rm drivers > fatal: not removing 'drivers' recursively without -r > > versus > > $ git rm -r drivers > [...removes everything under drivers/...] that is not what the man page is saying ... it refers to a "leading" directory name, not simply a directory name. if it should say simply "when a directory name is given", then it should be changed to say that. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git rm" seems to do recursive removal even without "-r"
On Sat, Oct 07, 2017 at 03:12:01PM -0400, Robert P. J. Day wrote: > ok, in that case, can you give me an example where "-r" makes a > difference, given that the man page refers to "a leading directory > name being given"? let's use as an example the linux kernel source, > where there are a *ton* of files named "Makefile" under the drivers/ > directory. > > should there be a difference between: > > $ git rm drivers/Makefile > $ git rm -r drivers/Makefile > > clearly, i have a "leading directory name" in both examples above ... > what should happen differently? Nothing, because there is nothing to recurse in the pathspecs you've given. Try: $ git rm drivers fatal: not removing 'drivers' recursively without -r versus $ git rm -r drivers [...removes everything under drivers/...] -Peff
Re: "git rm" seems to do recursive removal even without "-r"
On Sat, 7 Oct 2017, Todd Zullinger wrote: > Robert P. J. Day wrote: > > > > was just testing variations of "git rm", and man page claims: > > > > -r Allow recursive removal when a leading directory name is given. > > > > i tested this on the "pro git" book repo, which contains a top-level "book/" > > directory, and quite a number of "*.asc" files in various subdirectories one > > or more levels down. i ran: > > > > $ git rm book/\*.asc > > > > and it certainly seemed to delete *all* "*.asc" files no matter where they > > were under book/, even without the "-r" option. > > > > am i misunderstanding something? > > By shell-escaping the *, you're letting git perform the file glob. The > DISCUSSION section of git-rm(1) says "File globbing matches across directory > boundaries." > > # With bash performing file globbing > $ git rm -n Documentation/*.txt | wc -l > 199 > > # With git performing file globbing > $ git rm -n Documentation/\*.txt | wc -l > 578 ok, in that case, can you give me an example where "-r" makes a difference, given that the man page refers to "a leading directory name being given"? let's use as an example the linux kernel source, where there are a *ton* of files named "Makefile" under the drivers/ directory. should there be a difference between: $ git rm drivers/Makefile $ git rm -r drivers/Makefile clearly, i have a "leading directory name" in both examples above ... what should happen differently? rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git rm" seems to do recursive removal even without "-r"
Robert P. J. Day wrote: was just testing variations of "git rm", and man page claims: -r Allow recursive removal when a leading directory name is given. i tested this on the "pro git" book repo, which contains a top-level "book/" directory, and quite a number of "*.asc" files in various subdirectories one or more levels down. i ran: $ git rm book/\*.asc and it certainly seemed to delete *all* "*.asc" files no matter where they were under book/, even without the "-r" option. am i misunderstanding something? By shell-escaping the *, you're letting git perform the file glob. The DISCUSSION section of git-rm(1) says "File globbing matches across directory boundaries." # With bash performing file globbing $ git rm -n Documentation/*.txt | wc -l 199 # With git performing file globbing $ git rm -n Documentation/\*.txt | wc -l 578 -- Todd ~~ Whenever you find yourself on the side of the majority, it is time to pause and reflect. -- Mark Twain
"git rm" seems to do recursive removal even without "-r"
was just testing variations of "git rm", and man page claims: -r Allow recursive removal when a leading directory name is given. i tested this on the "pro git" book repo, which contains a top-level "book/" directory, and quite a number of "*.asc" files in various subdirectories one or more levels down. i ran: $ git rm book/\*.asc and it certainly seemed to delete *all* "*.asc" files no matter where they were under book/, even without the "-r" option. am i misunderstanding something? rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
[PATCH 2/2] fast-export: DIFF_STATUS_RENAMED instead of 'R'
Minor change to be consistent with the rest of the fast-export code. DIFF_STATUS_RENAMED is defined as 'R'. Signed-off-by: Miguel Torroja --- builtin/fast-export.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index a3ab7da..4d39324 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -280,7 +280,7 @@ static int diff_type_cmp(const void *a_, const void *b_) * appear in the output before it is renamed (e.g., when a file * was copied and renamed in the same commit). */ - return (a->status == 'R') - (b->status == 'R'); + return (a->status == DIFF_STATUS_RENAMED) - (b->status == DIFF_STATUS_RENAMED); } static void print_path_1(const char *path) -- 2.1.4
[PATCH 2/2] fast-export: DIFF_STATUS_RENAMED instead of 'R'
Minor change to be consistent with the rest of the fast-export code. DIFF_STATUS_RENAMED is defined as 'R'. Signed-off-by: Miguel Torroja --- builtin/fast-export.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index a3ab7da..4d39324 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -280,7 +280,7 @@ static int diff_type_cmp(const void *a_, const void *b_) * appear in the output before it is renamed (e.g., when a file * was copied and renamed in the same commit). */ - return (a->status == 'R') - (b->status == 'R'); + return (a->status == DIFF_STATUS_RENAMED) - (b->status == DIFF_STATUS_RENAMED); } static void print_path_1(const char *path) -- 2.1.4
[PATCH 2/2] fast-export: DIFF_STATUS_RENAMED instead of 'R'
Minor change to be consistent with the rest of the fast-export code. DIFF_STATUS_RENAMED is defined as 'R'. Signed-off-by: Miguel Torroja --- builtin/fast-export.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index a3ab7da..4d39324 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -280,7 +280,7 @@ static int diff_type_cmp(const void *a_, const void *b_) * appear in the output before it is renamed (e.g., when a file * was copied and renamed in the same commit). */ - return (a->status == 'R') - (b->status == 'R'); + return (a->status == DIFF_STATUS_RENAMED) - (b->status == DIFF_STATUS_RENAMED); } static void print_path_1(const char *path) -- 2.1.4
Re: fatal: bad revision 'git rm -r --ignore-unmatch -- folder'
I've finally found the correct command after some significant research:
git filter-branch --tag-name-filter cat --index-filter "git rm -r
--cached --ignore-unmatch ${file_path}/${file_name}" --prune-empty
--force -- --all
On Fri, Jan 20, 2017 at 4:23 PM, jean-christophe manciot
wrote:
> I've finally found the correct command after some significant research:
> git filter-branch --tag-name-filter cat --index-filter "git rm -r --cached
> --ignore-unmatch ${file_path}/${file_name}" --prune-empty --force -- --all
>
> On Thu, Jan 19, 2017 at 9:42 AM, jean-christophe manciot
> wrote:
>>
>> Also some context information:
>> Ubuntu 16.10 4.8
>> git 2.11.0
>>
>> On Thu, Jan 19, 2017 at 9:32 AM, jean-christophe manciot
>> wrote:
>> > In case you were wondering whether these files were tracked or not:
>> >
>> > git-Games# git ls-files Ubuntu/16.04
>> > Ubuntu/16.04/residualvm_0.3.0~git-1_amd64.deb
>> > Ubuntu/16.04/scummvm-data_1.8.0_all.deb
>> > Ubuntu/16.04/scummvm-data_1.9.0_all.deb
>> > Ubuntu/16.04/scummvm-dbgsym_1.8.2~git20160821.bad86050_amd64.deb
>> > Ubuntu/16.04/scummvm-dbgsym_1.9.0_amd64.deb
>> > Ubuntu/16.04/scummvm_1.8.0_amd64.deb
>> > Ubuntu/16.04/scummvm_1.9.0_amd64.deb
>> >
>> > On Tue, Jan 17, 2017 at 4:30 PM, jean-christophe manciot
>> > wrote:
>> >> Hi there,
>> >>
>> >> I'm trying to purge a complete folder and its files from the
>> >> repository history with:
>> >>
>> >> git-Games# git filter-branch 'git rm -r --ignore-unmatch --
>> >> Ubuntu/16.04/' --tag-name-filter cat -- --all HEAD
>> >> fatal: bad revision 'git rm -r --ignore-unmatch -- Ubuntu/16.04/'
>> >>
>> >> git does not find the folder although it's there:
>> >>
>> >> git-Games# ll Ubuntu/16.04/
>> >> total 150316
>> >> drwxr-x--- 2 actionmystique actionmystique 4096 Nov 19 13:40 ./
>> >> drwxr-x--- 4 actionmystique actionmystique 4096 Oct 30 14:02 ../
>> >> -rwxr-x--- 1 actionmystique actionmystique 2190394 May 9 2016
>> >> residualvm_0.3.0~git-1_amd64.deb*
>> >> ...
>> >> -rw-r--r-- 1 actionmystique actionmystique 67382492 Oct 13 09:15
>> >> scummvm-dbgsym_1.9.0_amd64.deb
>> >>
>> >> What is going on?
>> >>
>> >> --
>> >> Jean-Christophe
>> >
>> >
>> >
>> > --
>> > Jean-Christophe
>>
>>
>>
>> --
>> Jean-Christophe
>
>
>
>
> --
> Jean-Christophe
--
Jean-Christophe
Re: fatal: bad revision 'git rm -r --ignore-unmatch -- folder'
Also some context information: Ubuntu 16.10 4.8 git 2.11.0 On Thu, Jan 19, 2017 at 9:32 AM, jean-christophe manciot wrote: > In case you were wondering whether these files were tracked or not: > > git-Games# git ls-files Ubuntu/16.04 > Ubuntu/16.04/residualvm_0.3.0~git-1_amd64.deb > Ubuntu/16.04/scummvm-data_1.8.0_all.deb > Ubuntu/16.04/scummvm-data_1.9.0_all.deb > Ubuntu/16.04/scummvm-dbgsym_1.8.2~git20160821.bad86050_amd64.deb > Ubuntu/16.04/scummvm-dbgsym_1.9.0_amd64.deb > Ubuntu/16.04/scummvm_1.8.0_amd64.deb > Ubuntu/16.04/scummvm_1.9.0_amd64.deb > > On Tue, Jan 17, 2017 at 4:30 PM, jean-christophe manciot > wrote: >> Hi there, >> >> I'm trying to purge a complete folder and its files from the >> repository history with: >> >> git-Games# git filter-branch 'git rm -r --ignore-unmatch -- >> Ubuntu/16.04/' --tag-name-filter cat -- --all HEAD >> fatal: bad revision 'git rm -r --ignore-unmatch -- Ubuntu/16.04/' >> >> git does not find the folder although it's there: >> >> git-Games# ll Ubuntu/16.04/ >> total 150316 >> drwxr-x--- 2 actionmystique actionmystique 4096 Nov 19 13:40 ./ >> drwxr-x--- 4 actionmystique actionmystique 4096 Oct 30 14:02 ../ >> -rwxr-x--- 1 actionmystique actionmystique 2190394 May 9 2016 >> residualvm_0.3.0~git-1_amd64.deb* >> ... >> -rw-r--r-- 1 actionmystique actionmystique 67382492 Oct 13 09:15 >> scummvm-dbgsym_1.9.0_amd64.deb >> >> What is going on? >> >> -- >> Jean-Christophe > > > > -- > Jean-Christophe -- Jean-Christophe
Re: fatal: bad revision 'git rm -r --ignore-unmatch -- folder'
In case you were wondering whether these files were tracked or not: git-Games# git ls-files Ubuntu/16.04 Ubuntu/16.04/residualvm_0.3.0~git-1_amd64.deb Ubuntu/16.04/scummvm-data_1.8.0_all.deb Ubuntu/16.04/scummvm-data_1.9.0_all.deb Ubuntu/16.04/scummvm-dbgsym_1.8.2~git20160821.bad86050_amd64.deb Ubuntu/16.04/scummvm-dbgsym_1.9.0_amd64.deb Ubuntu/16.04/scummvm_1.8.0_amd64.deb Ubuntu/16.04/scummvm_1.9.0_amd64.deb On Tue, Jan 17, 2017 at 4:30 PM, jean-christophe manciot wrote: > Hi there, > > I'm trying to purge a complete folder and its files from the > repository history with: > > git-Games# git filter-branch 'git rm -r --ignore-unmatch -- > Ubuntu/16.04/' --tag-name-filter cat -- --all HEAD > fatal: bad revision 'git rm -r --ignore-unmatch -- Ubuntu/16.04/' > > git does not find the folder although it's there: > > git-Games# ll Ubuntu/16.04/ > total 150316 > drwxr-x--- 2 actionmystique actionmystique 4096 Nov 19 13:40 ./ > drwxr-x--- 4 actionmystique actionmystique 4096 Oct 30 14:02 ../ > -rwxr-x--- 1 actionmystique actionmystique 2190394 May 9 2016 > residualvm_0.3.0~git-1_amd64.deb* > ... > -rw-r--r-- 1 actionmystique actionmystique 67382492 Oct 13 09:15 > scummvm-dbgsym_1.9.0_amd64.deb > > What is going on? > > -- > Jean-Christophe -- Jean-Christophe
Re: fatal: bad revision 'git rm -r --ignore-unmatch -- folder'
On Tue, Jan 17, 2017 at 04:30:48PM +0100, jean-christophe manciot wrote: > I'm trying to purge a complete folder and its files from the > repository history with: > > git-Games# git filter-branch 'git rm -r --ignore-unmatch -- > Ubuntu/16.04/' --tag-name-filter cat -- --all HEAD > fatal: bad revision 'git rm -r --ignore-unmatch -- Ubuntu/16.04/' Did you forget "--tree-filter" or "--index-filter" before the "git rm" parameter? Without an option it will be interpreted as a refname, which it obviously isn't. -Peff
fatal: bad revision 'git rm -r --ignore-unmatch -- folder'
Hi there, I'm trying to purge a complete folder and its files from the repository history with: git-Games# git filter-branch 'git rm -r --ignore-unmatch -- Ubuntu/16.04/' --tag-name-filter cat -- --all HEAD fatal: bad revision 'git rm -r --ignore-unmatch -- Ubuntu/16.04/' git does not find the folder although it's there: git-Games# ll Ubuntu/16.04/ total 150316 drwxr-x--- 2 actionmystique actionmystique 4096 Nov 19 13:40 ./ drwxr-x--- 4 actionmystique actionmystique 4096 Oct 30 14:02 ../ -rwxr-x--- 1 actionmystique actionmystique 2190394 May 9 2016 residualvm_0.3.0~git-1_amd64.deb* ... -rw-r--r-- 1 actionmystique actionmystique 67382492 Oct 13 09:15 scummvm-dbgsym_1.9.0_amd64.deb What is going on? -- Jean-Christophe
Re: [PATCH v2] git-p4: do not pass '-r 0' to p4 commands
On 3 January 2017 at 20:02, Ori Rawlings wrote: > Looks good to me. And me. > > > Ori Rawlings
Re: [PATCH v2] git-p4: do not pass '-r 0' to p4 commands
Looks good to me. Ori Rawlings
[PATCH v2] git-p4: do not pass '-r 0' to p4 commands
git-p4 crashes when used with a very old p4 client version that does not support the '-r ' option in its commands. Allow making git-p4 work with old p4 clients by setting git-p4.retries to 0. Alternatively git-p4.retries could be made opt-in. But since only very old, barely maintained p4 versions don't support the '-r' option, the setting-retries-to-0 workaround would do. The "-r retries" option is present in Perforce 2012.2 Command Reference, but absent from Perforce 2012.1 Command Reference. Signed-off-by: Igor Kushnir --- Documentation/git-p4.txt | 2 ++ git-p4.py| 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt index bae862ddc..7436c64a9 100644 --- a/Documentation/git-p4.txt +++ b/Documentation/git-p4.txt @@ -479,6 +479,8 @@ git-p4.client:: git-p4.retries:: Specifies the number of times to retry a p4 command (notably, 'p4 sync') if the network times out. The default value is 3. + Set the value to 0 to disable retries or if your p4 version + does not support retries (pre 2012.2). Clone and sync variables diff --git a/git-p4.py b/git-p4.py index 22e3f57e7..7bda915bd 100755 --- a/git-p4.py +++ b/git-p4.py @@ -83,7 +83,9 @@ def p4_build_cmd(cmd): if retries is None: # Perform 3 retries by default retries = 3 -real_cmd += ["-r", str(retries)] +if retries > 0: +# Provide a way to not pass this option by setting git-p4.retries to 0 +real_cmd += ["-r", str(retries)] if isinstance(cmd,basestring): real_cmd = ' '.join(real_cmd) + ' ' + cmd -- 2.11.0
Re: [PATCH] git-p4: do not pass '-r 0' to p4 commands
> On 29 Dec 2016, at 10:05, Igor Kushnir wrote: > > git-p4 crashes when used with a very old p4 client version > that does not support the '-r ' option in its commands. > > Allow making git-p4 work with old p4 clients by setting git-p4.retries to 0. > > Alternatively git-p4.retries could be made opt-in. > But since only very old, barely maintained p4 versions don't support > the '-r' option, the setting-retries-to-0 workaround would do. > > The "-r retries" option is present in Perforce 2012.2 Command Reference, > but absent from Perforce 2012.1 Command Reference. Thanks for this workaround! > Signed-off-by: Igor Kushnir > --- > Documentation/git-p4.txt | 1 + > git-p4.py| 4 +++- > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt > index bae862ddc..f4f1be5be 100644 > --- a/Documentation/git-p4.txt > +++ b/Documentation/git-p4.txt > @@ -479,6 +479,7 @@ git-p4.client:: > git-p4.retries:: > Specifies the number of times to retry a p4 command (notably, > 'p4 sync') if the network times out. The default value is 3. > + '-r 0' is not passed to p4 commands if this option is set to 0. At this point in the docs we have never talked about the "-r" flag and I would argue it is an "implementation detail". Therefore, I would prefer something like: "Set the value to 0 if want to disable retries or if your p4 version does not support retries (pre 2012.2)." > > Clone and sync variables > > diff --git a/git-p4.py b/git-p4.py > index 22e3f57e7..e5a9e1cce 100755 > --- a/git-p4.py > +++ b/git-p4.py > @@ -83,7 +83,9 @@ def p4_build_cmd(cmd): > if retries is None: > # Perform 3 retries by default > retries = 3 > -real_cmd += ["-r", str(retries)] > +if retries != 0: How about "retries > 0"? > +# Provide a way to not pass this option by setting git-p4.retries to > 0 > +real_cmd += ["-r", str(retries)] > > if isinstance(cmd,basestring): > real_cmd = ' '.join(real_cmd) + ' ' + cmd > -- > 2.11.0 >
