Re: [PATCH v7 07/17] # This is a combination of 2 commits. # This is the 1st commit message:
On Thu, Apr 19, 2018 at 8:20 AM, Johannes Schindelin wrote: > # This is a combination of 2 commits. # This is the 1st commit message: Botched squash/fixup? > sequencer: introduce the `merge` command > > This patch is part of the effort to reimplement `--preserve-merges` with > a substantially improved design, a design that has been developed in the > Git for Windows project to maintain the dozens of Windows-specific patch > series on top of upstream Git. > [...] > Signed-off-by: Johannes Schindelin > > # The commit message #2 will be skipped: > > # fixup! sequencer: introduce the `merge` command Bloop.
Re: [PATCH v2 1/2] completion: stop showing 'save' for stash by default
On Fri, Apr 20, 2018 at 1:25 AM, Thomas Gummerer wrote: > The 'save' subcommand in git stash has been deprecated in > fd2ebf14db ("stash: mark "git stash save" deprecated in the man page", > 2017-10-22). > > Stop showing it when the users enters 'git stash ' or 'git stash > s'. Keep showing it however when the user enters 'git stash sa' > or any more characters of the 'save' subcommand. I don't think this is worth it. You only save two keystrokes for 've' and already waste one on . -- Duy
Re: [BUG] Git fast-export with import marks file omits merge commits
On 20 April 2018 at 00:48, Junio C Hamano wrote: > Isaac Chou writes: > >> I inspected the source code (builtin/fast-export.c) for the >> fast-export issue I encountered, and it looks like the merge >> commit is discarded too early by the call to object_array_pop() >> after only one of the two UNSHOWN parents is processed in the >> method handle_tail(). The poped merge commit still has one >> UNSHOWN parent, therefore it is not processed and is lost in the >> output. Can someone advise me on how to submit a code change or >> bug report in order to get the fix into the code base? > > There indeed are some differences between v2.14 and v2.15 around the > code that returns early when has_unshown_parent() says "yes" [*1*], > but the decision to return early when the function says "yes" hasn't > changed between that timeperiod---it dates back to f2dc849e ("Add > 'git fast-export', the sister of 'git fast-import'", 2007-12-02), > i.e. the very beginning of the program's life. > > I'll CC those who wrote the original and b3e8ca89 ("fast-export: do > not copy from modified file", 2017-09-20) and 71992039 > ("object_array: add and use `object_array_pop()`", 2017-09-23), > which are the only two commits that touch the surrounding area > during that timeperiod, to ask if something jumps at them. > > Thanks. > > > [Footnotes] > > *1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c' > reads like so: > > diff --git a/builtin/fast-export.c b/builtin/fast-export.c > index d412c0a8f3..2fb60d6d48 100644 > --- a/builtin/fast-export.c > +++ b/builtin/fast-export.c > ... > @@ -630,15 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len) > return strbuf_detach(&out, len); > } > > -static void handle_tail(struct object_array *commits, struct rev_info *revs) > +static void handle_tail(struct object_array *commits, struct rev_info *revs, > + struct string_list *paths_of_changed_objects) > { > struct commit *commit; > while (commits->nr) { > - commit = (struct commit *)commits->objects[commits->nr - > 1].item; > + commit = (struct commit *)object_array_pop(commits); > if (has_unshown_parent(commit)) > return; > - handle_commit(commit, revs); > - commits->nr--; > + handle_commit(commit, revs, paths_of_changed_objects); > } > } Indeed. This looks wrong and the guilty person would be me. If my 71992039 ("object_array: add and use `object_array_pop()`", 2017-09-23) would instead have done something like s/commits->nr--/(void)object_array_pop(commits)/ it would not have screwed up as much. It could also use a peek+pop-pattern. Isaac, are you up for submitting a patch? Just let me know if you encounter any issues. Otherwise, I can submit a patch shortly since I was the one who dropped the ball originally. Thanks for diagnosing this. Martin
Re: [PATCH v10 00/36] Add directory rename detection to git
Elijah Newren writes: > On Thu, Apr 19, 2018 at 10:57 AM, Elijah Newren wrote: >> This series is a reboot of the directory rename detection series that was >> merged to master and then reverted due to the final patch having a buggy >> can-skip-update check, as noted at >> https://public-inbox.org/git/xmqqmuya43cs@gitster-ct.c.googlers.com/ >> This series based on top of master. > > ...and merges cleanly to next but apparently has some minor conflicts > with both ds/lazy-load-trees and ps/test-chmtime-get from pu. > > What's the preferred way to resolve this? Rebase and resubmit my > series on pu, or something else? The series as-is is fine, I think, from the maintainer's point of view. Thanks.
Re: [PATCH v2 0/2] completion: improvements for git stash
Thomas Gummerer writes: > I didn't find a good way to implement "reluctant completion" (I'm also > by no means an expert in bash completion, so there may well be a way I > couldn't find by googl'ing around), so I left that out of this > series. > > I don't think it's strictly necessary for the deprecation either, as > we can just print a warning message when the user actually uses 'git > stash save' at some point, which would make a message printed when > using the completion redundant anyway. I feel like that warning > message is not something we're quite ready for yet and I'd rather wait > a few more releases before doing that though. Yeah, I agree that what you outined above is quite sensible. Thanks, will queue.
Re: [PATCH v2 3/7] Add a test for `git replace --convert-graft-file`
Johannes Schindelin writes: > The proof, as the saying goes, lies in the pudding. So here is a > regression test that not only demonstrates what the option is supposed to > accomplish, but also demonstrates that it does accomplish it. The above spreads the misconception that the value of the test is "what I wrote works, look here". It is not. "Here is how this thing is supposed to work. You are free to improve it, but do not break the basic promises these tests outline" to protect the resulting system is. > Signed-off-by: Johannes Schindelin > --- > t/t6050-replace.sh | 20 > 1 file changed, 20 insertions(+) > > diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh > index c630aba657e..77ded6df653 100755 > --- a/t/t6050-replace.sh > +++ b/t/t6050-replace.sh > @@ -444,4 +444,24 @@ test_expect_success GPG '--graft on a commit with a > mergetag' ' > git replace -d $HASH10 > ' > > +test_expect_success '--convert-graft-file' ' > + : add and convert graft file && > + printf "%s\n%s %s\n%s\n" \ > + $(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \ > + >.git/info/grafts && I find the above much less readbale than something like { git rev-parse HEAD^^ git rev-parse HEAD^ HEAD^^ git rev-parse HEAD^2 } >.git/info/grafts because printf formatting string must be deciphered and then matched against the order and number of rev-parse arguments (and printf's ability to happily accept more args than the placeholders does not help in readablity---the reader needs to verify that the code is not doing anything overly clever exploiting that ability) before I can figure out who's parent of whom. Of course, it saves a few spawning of subprocesses; I am not sure if that is worth the loss of readability in this case, though. > + git replace --convert-graft-file && > + test_path_is_missing .git/info/grafts && Successful conversion exits with success, and removes the grafts file. Good. > + : verify that the history is now "grafted" && > + git rev-list HEAD >out && > + test_line_count = 4 out && We are pretending that HEAD^ has only one parent HEAD^^ and that HEAD^^ and HEAD^2 are roots with the above graft. We want the resulting "apparent" history to also show that fact, and the traversal should stop after counting HEAD, HEAD^, HEAD^2 (which is root), and HEAD^^ (which is also root). Good. > + : create invalid graft file and verify that it is not deleted && > + test_when_finished "rm -f .git/info/grafts" && > + echo $EMPTY_BLOB $EMPTY_TREE >.git/info/grafts && > + test_must_fail git replace --convert-graft-file 2>err && > + grep "$EMPTY_BLOB $EMPTY_TREE" err && > + grep "$EMPTY_BLOB $EMPTY_TREE" .git/info/grafts > +' > + > test_done
Re: [PATCH v2 2/7] replace: introduce --convert-graft-file
Johannes Schindelin writes: > This option is intended to help with the transition away from the > now-deprecated graft file. > > Signed-off-by: Johannes Schindelin > --- > Documentation/git-replace.txt | 11 +-- > builtin/replace.c | 59 ++- > 2 files changed, 66 insertions(+), 4 deletions(-) I expected you to remove convert-grafts-to-replace-refs.sh from the contrib/ section in the same patch, actually. FWIW, I think it is a much better approach to give the first-class UI for transition like this patch does than "go fish for a good way to transition yourself, we may have something useful in contrib/", which is what we had so far. > diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt > ... > @@ -87,9 +88,13 @@ OPTIONS > content as except that its parents will be > [...] instead of 's parents. A replacement ref > is then created to replace with the newly created > - commit. See contrib/convert-grafts-to-replace-refs.sh for an > - example script based on this option that can convert grafts to > - replace refs. > + commit. Use `--convert-graft-file` to convert a > + `$GIT_DIR/info/grafts` file use replace refs instead. > + Nice. > diff --git a/builtin/replace.c b/builtin/replace.c > index 43264f0998e..4cdc00a96df 100644 > --- a/builtin/replace.c > +++ b/builtin/replace.c > @@ -20,6 +20,7 @@ static const char * const git_replace_usage[] = { > N_("git replace [-f] "), > N_("git replace [-f] --edit "), > N_("git replace [-f] --graft [...]"), > + N_("git replace [-f] --convert-graft-file"), > N_("git replace -d ..."), > N_("git replace [--format=] [-l []]"), > NULL > @@ -423,6 +424,53 @@ static int create_graft(int argc, const char **argv, int > force) > return replace_object_oid(old_ref, &old_oid, "replacement", &new_oid, > force); > } > > +static int convert_graft_file(int force) > +{ > + const char *graft_file = get_graft_file(); > + FILE *fp = fopen_or_warn(graft_file, "r"); > + struct strbuf buf = STRBUF_INIT, err = STRBUF_INIT; > + struct argv_array args = ARGV_ARRAY_INIT; > + > + if (!fp) > + return -1; Returning silently is fine as fopen_or_warn() would have said something already. Good. > + while (strbuf_getline(&buf, fp) != EOF) { > + int i = 0, j; > + > + while (i != buf.len) { > + char save; > + > + for (j = i; j < buf.len && !isspace(buf.buf[j]); j++) > + ; /* look further */ > + save = buf.buf[j]; > + buf.buf[j] = '\0'; > + argv_array_push(&args, buf.buf + i); > + buf.buf[j] = save; It's a shame that we do not have a helper that splits the contents of a strbuf at SP and shove the result into an argv_array(). [*1*] *1* There is one that splits into an array of strbuf but the point of splitting is often that these split pieces are the final thing we want, and placing them in separate strbuf (whose strength is that contents are easily manipulatable) is pointless. > + > + while (j < buf.len && isspace(buf.buf[j])) > + j++; > + i = j; One difference I notice while comparing this with what is done by contrib/convert-grafts-to-replace-refs.sh is that this does not skip a line that begins with # or SP. I offhand do not know what the point of skipping a line that begins with a SP, but I suspect that skipping a line that begins with "#" is a desirable thing to do, because commit.c::read_graft_line() does know that such a line is a valid comment. > + } > + > + if (create_graft(args.argc, args.argv, force)) > + strbuf_addf(&err, "\n\t%s", buf.buf); > + > + argv_array_clear(&args); > + strbuf_reset(&buf); Strictly speaking, this reset is redundant, as getline() will always stuff the line into a fresh buffer (and after the loop there correctly is a release). > + } > + > + strbuf_release(&buf); > + argv_array_clear(&args); > + > + if (!err.len) > + return unlink_or_warn(graft_file); > + warning(_("could not convert the following graft(s):\n%s"), err.buf); > + strbuf_release(&err); commit.c::read_graft_file() seems to ignore a broken graft line and salvages other lines, and this one follows suit, which is good. The remaining die() I pointed out in 1/2 can safely be turned into return error() for this caller (I didn't check for existing callers, though) and would automatically do the right thing. The real consumer of the graft file, commit.c::read_graft_line(), shows an error when oid cannot be parsed, and the above code, when create_graft() is updated to return an error instead of dying, would append the problematic record to buf.buf in the code above. Looki
Re: [PATCH v2 1/7] replace: "libify" create_graft()
Johannes Schindelin writes: > It is quite convenient to simply die() in builtins, in the absence of > proper exception handling, because it allows us to just go belly up > without having to implement error handling chains. > > Of course, for reusable library functions, this is a big no-no, so we > (try to) restrict the usage of die() to one-shot commands, i.e. places > where we know that the caller does not want to, say, give the user a > useful high-level error message, i.e. a message that the function calling > die() could not possibly know. > > The problem with this reasoning is that sooner or later, pretty much all > useful functions will *need* to be libified: the more useful a function, > the more likely it is to be called from a call chain where the outer > function implements a high-level operation that needs to provide > additional advice to the user in case of failure. > > This is the case here: the create_graft() function is useful enough to be > called in a loop, say, in the upcoming patch to convert a graft file in > one fell swoop. Therefore, this function must not be allowed to die(), nor > any of its callees. All of the first three paragraphs are already widely known to the project, and I do not think you need to verbosely repeat it, if brevity demands it. One thing that the proposed log message for this step would be far more useful to say is that the current callers of create_graft() is fine with the behaviour change (i.e. prepared to act on an error return). Also it may want to say why the other die() we see in this function in the pre-context is OK. I actually think it is not OK and should return error(not a valid object name) the same way (I suspect that this is mere omission/mistake, and you did not intend to leave it dying). As long as the caller is prepared to deal with an error return, that is, which was the previous point. FWIW, I fully agree with the goal of making this (or other pieces that would be useful if they were reusable) reusable. > Signed-off-by: Johannes Schindelin > --- > builtin/replace.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/builtin/replace.c b/builtin/replace.c > index 935647be6bd..43264f0998e 100644 > --- a/builtin/replace.c > +++ b/builtin/replace.c > @@ -395,7 +395,9 @@ static int create_graft(int argc, const char **argv, int > force) > > if (get_oid(old_ref, &old_oid) < 0) > die(_("Not a valid object name: '%s'"), old_ref); > - commit = lookup_commit_or_die(&old_oid, old_ref); > + commit = lookup_commit_reference(&old_oid); > + if (!commit) > + return error(_("could not parse %s"), old_ref); > > buffer = get_commit_buffer(commit, &size); > strbuf_add(&buf, buffer, size);
Re: Silly "git gc" UI issue.
Simon Ruderich writes: > On Thu, Apr 19, 2018 at 10:52:47AM +0900, Junio C Hamano wrote: >> It turns out that prune silently goes away given a bad expiry >> >> $ git prune --expire=nyah ; echo $? >> 129 > > I noticed that git log --since/--after/--before/--until have a > similar behavior and ignore date parsing errors in those options > completely. Is this expected or should we warn the user with > something like the following? I do not have a strong opinion on this, because I would expect that "git log --since=nyah" to do whatever random things it may want to do. But I suspect I am a minority, and if we were to change the established behaviour, I agree that erroring out using approxidate_careful() is the right direction to go in. Thanks. > diff --git a/revision.c b/revision.c > index 4e0e193e57..e5ba6c7dfc 100644 > --- a/revision.c > +++ b/revision.c > @@ -1794,19 +1794,31 @@ static int handle_revision_opt(struct rev_info *revs, > int argc, const char **arg > revs->max_age = atoi(optarg); > return argcount; > } else if ((argcount = parse_long_opt("since", argv, &optarg))) { > - revs->max_age = approxidate(optarg); > + int err = 0; > + revs->max_age = approxidate_careful(optarg, &err); > + if (err) > + return error("--since: invalid time '%s'", optarg); > return argcount; > } else if ((argcount = parse_long_opt("after", argv, &optarg))) { > - revs->max_age = approxidate(optarg); > + int err = 0; > + revs->max_age = approxidate_careful(optarg, &err); > + if (err) > + return error("--after: invalid time '%s'", optarg); > return argcount; > } else if ((argcount = parse_long_opt("min-age", argv, &optarg))) { > revs->min_age = atoi(optarg); > return argcount; > } else if ((argcount = parse_long_opt("before", argv, &optarg))) { > - revs->min_age = approxidate(optarg); > + int err = 0; > + revs->min_age = approxidate_careful(optarg, &err); > + if (err) > + return error("--before: invalid time '%s'", optarg); > return argcount; > } else if ((argcount = parse_long_opt("until", argv, &optarg))) { > - revs->min_age = approxidate(optarg); > + int err = 0; > + revs->min_age = approxidate_careful(optarg, &err); > + if (err) > + return error("--until: invalid time '%s'"); > return argcount; > } else if (!strcmp(arg, "--first-parent")) { > revs->first_parent_only = 1; > > Regards > Simon
Re: [RFC WIP PATCH] merge: implement -s theirs -X N
Ævar Arnfjörð Bjarmason writes: > On Thu, Apr 19 2018, Junio C. Hamano wrote: > >> This question has nothing to do with your "-s theirs" but let me see >> if I got the above correctly. Suppose you have a deployed branch >> (say, "prod"), all developments happen on "master" elsewhere that >> can be seen as "origin/master", so you may have a few fixes that is >> not yet in "prod" you would want to cherry-pick from origin/master. >> >> $ git checkout prod >> $ git cherry-pick origin/master~2 >> $ git cherry-pick origin/master >> >> Let's say that "master" had a fix at HEAD~2, HEAD~1 is a feature >> enhancement that is not yet ready for "prod", and HEAD is another >> fix. Up to this point you successfully back-ported the fixes to >> "prod". >> >> Then you do merge the tip into "master", i.e. >> >> $ git checkout origin/master && git merge -s ours prod >> $ git push origin HEAD:master >> $ git checkout prod >> >> to make sure that the "master" at the source of truth knows that >> it already has what our "prod" with these two cherry-picks have. >> >> Is that what is going on here? >> >> I am just wondering what would and should happen to the non-fix >> commit in the middle in the above example. Perhaps your workflow >> automatically does the right thing to it, perhaps not. >> >> >> [Footnote] >> ... > Yeah this -s theirs is redundant to just doing it the other way around > as you describe. > ... Heh, you responded to a much less relevant footnote without addressing the main part of the message which was a more interesting question to me ;-)
Re: [PATCH] git-send-email: Cc more people
Mathieu Desnoyers writes: >>> I'd further say that these new CC-sources should be disabled by >>> default and made opt-in to avoid surprising existing users. >> >> But I disagree with this. The current behaviour is surprising to >> existing users, to the point where people are writing their own scripts >> to replace git send-email (which seems crazy to me). > > We could perhaps go with a whitelist approach. The four > main match I would be tempted to add are: Acked-by, Reported-by, > Reviewed-by, and Tested-by. A tool that suddenly starts sending e-mails to more addresses without letting the end-users know when and why the change in behaviour happened is a source of irritated "somebody made a stupid change to git-send-email without telling us that caused unwanted e-mails sent to unexpected places and embarrassed me" bug reports. I do agree with a whitelist approach from that point of view, and in the initial rollout of the feature, that whitelist should be limited to what we already send out. The users who learn about this new feature can opt into whitelisting the common 4 above before we enable them by default. FWIW, I personally think these will be a sensible default (in addition to what we already Cc). I however prefer an approach to introduce these more gradually.
[PATCH v2 2/2] completion: make stash -p and alias for stash push -p
We define 'git stash -p' as an alias for 'git stash push -p' in the manpage. Do the same in the completion script, so all options that can be given to 'git stash push' are being completed when the user is using 'git stash -p --'. Currently the only additional option the user will get is '--message', but there may be more in the future. Signed-off-by: Thomas Gummerer --- contrib/completion/git-completion.bash | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 9a95b3b7b1..adb6516b6d 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -2776,6 +2776,9 @@ _git_stash () local save_opts='--all --keep-index --no-keep-index --quiet --patch --include-untracked' local subcommands='push list show apply clear drop pop create branch' local subcommand="$(__git_find_on_cmdline "$subcommands save")" + if [ -n "$(__git_find_on_cmdline "-p")" ]; then + subcommand="push" + fi if [ -z "$subcommand" ]; then case "$cur" in --*) -- 2.17.0.252.gfe0a9eaf31
[PATCH v2 1/2] completion: stop showing 'save' for stash by default
The 'save' subcommand in git stash has been deprecated in fd2ebf14db ("stash: mark "git stash save" deprecated in the man page", 2017-10-22). Stop showing it when the users enters 'git stash ' or 'git stash s'. Keep showing it however when the user enters 'git stash sa' or any more characters of the 'save' subcommand. This is designed to not encourage users to use 'git stash save', but still leaving the completion option once it's clear that's what the user means. Signed-off-by: Thomas Gummerer --- contrib/completion/git-completion.bash | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index a757073945..9a95b3b7b1 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -2774,13 +2774,18 @@ _git_show_branch () _git_stash () { local save_opts='--all --keep-index --no-keep-index --quiet --patch --include-untracked' - local subcommands='push save list show apply clear drop pop create branch' - local subcommand="$(__git_find_on_cmdline "$subcommands")" + local subcommands='push list show apply clear drop pop create branch' + local subcommand="$(__git_find_on_cmdline "$subcommands save")" if [ -z "$subcommand" ]; then case "$cur" in --*) __gitcomp "$save_opts" ;; + sa*) + if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then + __gitcomp "save" + fi + ;; *) if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then __gitcomp "$subcommands" -- 2.17.0.252.gfe0a9eaf31
[PATCH v2 0/2] completion: improvements for git stash
Previous round was at <20180417212945.24002-1-t.gumme...@gmail.com>. Thanks Junio for your input on the previous round. This round drops what was 1/3 in the previous round. We keep completing the options for 'git stash save', so calling the variable 'save_opts' and defining what would be 'push_opts' as 'save_opts' + '--message' makes sense. 2/3 (now 1/2) was mostly rewritten. We now no longer suggest 'save' in 'git stash ', complete 'git stash s' will complete to 'git stash show', but 'git stash sa' (or longer) will keep completing to 'git stash save', as the user most likely already knows about 'git stash save', and wanted to type that. We also keep completing the options for 'git stash save' on 'git stash save --'. 3/3 (now 2/2) stays the same as in the previous round. I didn't find a good way to implement "reluctant completion" (I'm also by no means an expert in bash completion, so there may well be a way I couldn't find by googl'ing around), so I left that out of this series. I don't think it's strictly necessary for the deprecation either, as we can just print a warning message when the user actually uses 'git stash save' at some point, which would make a message printed when using the completion redundant anyway. I feel like that warning message is not something we're quite ready for yet and I'd rather wait a few more releases before doing that though. Thomas Gummerer (2): completion: stop showing 'save' for stash by default completion: make stash -p and alias for stash push -p contrib/completion/git-completion.bash | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) -- 2.17.0.252.gfe0a9eaf31
Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll
Kim Gybels writes: >> > In other words, you scolded Kim for something that this patch did not >> > introduce, but which was already there. > > I didn't feel scolded, just Junio raising a concern about maintainability of > the code. FWIW, I didn't mean to scold, either. Rather I was pointing out that the code already maintains the number of remaining children, which means that a more portable abstraction than poll(), if we desired to have one, would merely be one step away, as we already know that at least need that information to help Windows. > There is another issue with the existing code that this new > "xpoll" will need to take into account. If a SIGCHLD arrives > between the call to check_dead_children and poll, the poll will > not be interupted by it, resulting in the child not being reaped > until another child terminates or a client connects. Currently, > the effect is just a zombie process for a longer time, however, > the proposed patch (daemon: graceful shutdown of client > connection) relies on the cleanup to close the client connection. Good analysis. That consideration may mean that xpoll() as I suggested is useless as a possible more portable abstraction (or, "an abstraction that is implementable easily in both POSIX and non-POSIX world"), but I suspect we would still want to have an internal "portable" API that serves the purpose similar to how we wanted POSIX poll() to serve. The place the patch is touching is not the only place poll() is used in the codebase, and other places (and future ones we would add) may benefit from having one. Thanks for being constructive.
Re: [BUG] Git fast-export with import marks file omits merge commits
Isaac Chou writes: > I inspected the source code (builtin/fast-export.c) for the > fast-export issue I encountered, and it looks like the merge > commit is discarded too early by the call to object_array_pop() > after only one of the two UNSHOWN parents is processed in the > method handle_tail(). The poped merge commit still has one > UNSHOWN parent, therefore it is not processed and is lost in the > output. Can someone advise me on how to submit a code change or > bug report in order to get the fix into the code base? > > Thanks, > > Isaac > > -Original Message- > From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf > Of Isaac Chou > Sent: Monday, April 16, 2018 3:58 PM > To: git@vger.kernel.org > Subject: Git fast-export with import marks file omits merge commits > > Hello, > > I came across a change of behavior with Git version 2.15 and later > where the fast-export command would omit the merge commits. The > same use case works correctly with Git version 2.14 and older. > ... There indeed are some differences between v2.14 and v2.15 around the code that returns early when has_unshown_parent() says "yes" [*1*], but the decision to return early when the function says "yes" hasn't changed between that timeperiod---it dates back to f2dc849e ("Add 'git fast-export', the sister of 'git fast-import'", 2007-12-02), i.e. the very beginning of the program's life. I'll CC those who wrote the original and b3e8ca89 ("fast-export: do not copy from modified file", 2017-09-20) and 71992039 ("object_array: add and use `object_array_pop()`", 2017-09-23), which are the only two commits that touch the surrounding area during that timeperiod, to ask if something jumps at them. Thanks. [Footnotes] *1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c' reads like so: diff --git a/builtin/fast-export.c b/builtin/fast-export.c index d412c0a8f3..2fb60d6d48 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c ... @@ -630,15 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len) return strbuf_detach(&out, len); } -static void handle_tail(struct object_array *commits, struct rev_info *revs) +static void handle_tail(struct object_array *commits, struct rev_info *revs, + struct string_list *paths_of_changed_objects) { struct commit *commit; while (commits->nr) { - commit = (struct commit *)commits->objects[commits->nr - 1].item; + commit = (struct commit *)object_array_pop(commits); if (has_unshown_parent(commit)) return; - handle_commit(commit, revs); - commits->nr--; + handle_commit(commit, revs, paths_of_changed_objects); } }
Re: [BUG] Git fast-export with import marks file omits merge commits
Hi Isaac, On Thu, Apr 19, 2018 at 2:46 PM, Isaac Chou wrote: > I inspected the source code (builtin/fast-export.c) for the fast-export issue > I encountered, and it looks like the merge commit is discarded too early by > the call to object_array_pop() after only one of the two UNSHOWN parents is > processed in the method handle_tail(). The poped merge commit still has one > UNSHOWN parent, therefore it is not processed and is lost in the output. Can > someone advise me on how to submit a code change or bug report in order to > get the fix into the code base? Careful now, fast-export was also the location of my first patch. It's easy to get addicted to contributing changes to git. :-) Inside the git.git repository, there are two files that explain the basic process -- Documentation/SubmittingPatches and Documentation/CodingGuidelines. If they don't cover the process well, that's probably a bug itself, but feel free to ask on the list if you still run into questions. Those documents can be slighly overwhelming at first glance, but they've got good information. Elijah
Re: Bug Report - Pull remote branch does not retrieve new tags
Andrew, On Thu, Apr 19, 2018 at 6:55 AM, Andrew Ducker wrote: > > What happens: > When I create a new tag on the remote (changing nothing else) > "git pull origin master" produces the following: > From git.internal.company.com:team/testrepo >* branchmaster -> FETCH_HEAD > Already up-to-date. > > If I instead do a "git pull" I get: > From git.internal.company.com:team/testrepo >* [new tag] Testing11 -> Testing11 > Already up-to-date. > > What I think should happen: > The "git pull origin master" should retrieve the tag. > > This is with 2.16.2.windows.1, but also occurred on my previously installed > version (2.12.2.windows.2) > > My understanding is that "git pull" and "git pull $repo $currentbranch" > should function identically. > > Is this a bug, or am I misunderstanding the manual page? Looks like a misunderstanding, to me. Perhaps I can help clarify. "git pull" without arguments fetches from the "origin" repository using the configured "fetch" refspecs, which typically looks something like "fetch = +refs/heads/*:refs/remotes/origin/*". It _doesn't_ actually fetch all tags, but any tag referencing any object/commit included in the branches is brought along for the ride. This is documented on "git pull": --no-tags By default, tags that point at objects that are downloaded from the remote repository are fetched and stored locally. This option disables this automatic tag following. The default behavior for a remote may be specified with the remote..tagOpt setting. See git-config(1). By comparison, on your "git pull $repo $currentBranch", what you're calling "$currentBranch" is actually "[...]" from the documentation. In other words, by passing "master", you've told "git pull" to fetch _nothing but "master"_, ignoring the configured refspec(s). Additionally, since you haven't told "git pull" where to _put_ "master" once it's fetched, it writes it to "FETCH_HEAD". If you have a tracking branch setup, "git pull origin master" will also update the tracking branch. For example, the same command for me produces: $ git pull origin master >From ... * branchmaster -> FETCH_HEAD aca5eb0fef5..ad484477508 master -> origin/master As you can see, both FETCH_HEAD and origin/master were updated, since my local "master" tracks "origin"'s "master": [branch "master"] remote = origin merge = refs/heads/master Hope this helps! Bryan
RE: [BUG] Git fast-export with import marks file omits merge commits
I inspected the source code (builtin/fast-export.c) for the fast-export issue I encountered, and it looks like the merge commit is discarded too early by the call to object_array_pop() after only one of the two UNSHOWN parents is processed in the method handle_tail(). The poped merge commit still has one UNSHOWN parent, therefore it is not processed and is lost in the output. Can someone advise me on how to submit a code change or bug report in order to get the fix into the code base? Thanks, Isaac -Original Message- From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf Of Isaac Chou Sent: Monday, April 16, 2018 3:58 PM To: git@vger.kernel.org Subject: Git fast-export with import marks file omits merge commits Hello, I came across a change of behavior with Git version 2.15 and later where the fast-export command would omit the merge commits. The same use case works correctly with Git version 2.14 and older. Here is the detail of the use case: 0> git --version git version 2.16.2.windows.1 1> git init Initialized empty Git repository in c:/./.git/ 2> echo >> file.txt 3> git add file.txt 4> git commit -m "first commit" [master (root-commit) 711d4d5] first commit 1 file changed, 1 insertion(+) create mode 100644 file.txt 5> git checkout -b test Switched to a new branch 'test' 6> echo >> file.txt 7> git add file.txt 8> git commit -m "commit on test branch" [test 76d231c] commit on test branch 1 file changed, 1 insertion(+) 9> git checkout master Switched to branch 'master' 10> echo >> file.txt 11> git add file.txt 12> git commit -m "commit on master branch" [master 61c55fd] commit on master branch 1 file changed, 1 insertion(+) 13> git merge test Auto-merging file.txt CONFLICT (content): Merge conflict in file.txt Automatic merge failed; fix conflicts and then commit the result. 14> notepad file.txt 15> git add file.txt 16> git commit -m "merged with test branch" [master 1442e0e] merged with test branch 17> git log commit 1442e0ee728c831e74550329e39d27d4188b4422 (HEAD -> master) Merge: 61c55fd 76d231c Author: isaac <...> Date: Mon Apr 16 15:08:39 2018 -0400 merged with test branch commit 61c55fdb883fc403e63c91b49bc11bdade62b3e8 Author: isaac <...> Date: Mon Apr 16 15:07:41 2018 -0400 commit on master branch commit 76d231cdb12eb84f45abdebede06a56f912613d3 (test) Author: isaac <...> Date: Mon Apr 16 15:07:07 2018 -0400 commit on test branch commit 711d4d5781df41924421f8629af040e7c91a8d2e Author: isaac <...> Date: Mon Apr 16 15:06:07 2018 -0400 first commit 18> echo :1 711d4d5781df41924421f8629af040e7c91a8d2e > git-marks 19> cat git-marks :1 711d4d5781df41924421f8629af040e7c91a8d2e 20> git fast-export --use-done-feature --import-marks=git-marks refs/heads/master -- feature done blob mark :2 data 12 commit refs/heads/master mark :3 author isaac <...> 1523905627 -0400 committer isaac <...> 1523905627 -0400 data 22 commit on test branch from :1 M 100644 :2 file.txt blob mark :4 data 12 commit refs/heads/master mark :5 author isaac <...> 1523905661 -0400 committer isaac <...> 1523905661 -0400 data 24 commit on master branch from :1 M 100644 :4 file.txt done Thanks, Isaac
Re: [PATCH v3 1/1] perl: fix installing modules from contrib
Junio C Hamano on Thu, 2018/04/19 06:44: > Christian Hesse writes: > > > Commit 20d2a30f (Makefile: replace perl/Makefile.PL with simple make > > rules) removed a target that allowed Makefiles from contrib/ to get the > > correct install path. This introduces a new target for main Makefile and > > fixes installation for Mediawiki module. > > > > v2: Pass prefix as that can have influence as well, add single quotes > > for _SQ variant. > > v3: Rename target, add to .PHONY. > > > > Signed-off-by: Christian Hesse > > --- > > Thanks for rerolling. I should have made it a bit more clear that > the say-* thing was merely a personal preference "I would be writing > it that way if I were doing it", not a "You should write it this way > when working on this project". Well, it's you who maintains the code. So I am fine with whatever you prefer. ;) > I think .PHONY is still a good idea > to have, even for only its documentation value (it is unlikely that > anybody would create a file "perllibdir"). > > Let me queue this on top of the v2 queued in 'next' as an > incremental update. Thanks a lot! -- Best regards, Chris pgpeq183QArFF.pgp Description: OpenPGP digital signature
Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll
On (19/04/18 06:51), Junio C Hamano wrote: > Johannes Schindelin writes: > > In other words, you scolded Kim for something that this patch did not > > introduce, but which was already there. I didn't feel scolded, just Junio raising a concern about maintainability of the code. > > Unless I am misunderstanding violently what you say, that is, in which > > case I would like to ask for a clarification why this patch (which does > > not change a thing unless NO_POLL is defined!) must be rejected, and while > > at it, I would like to ask you how introducing a layer of indirection with > > a full new function that is at least moderately misleading (as it would be > > named xpoll() despite your desire that it should do things that poll() > > does *not* do) would be preferable to this here patch that changes but a > > few lines to introduce a regular heartbeat check for platforms that > > Our xwrite() and other xfoo() are to "fix" undesirable aspect of the > underlying pure POSIX API to make it more suitable for our codebase. > When pure POSIX poll() that requires the implementing or emulating > platform pays attention to the children being waited on is not > appropriate for the codepath we are using (i.e. the place where the > patch is touching), it would be in line to introduce a "fixed" API > that allows us to pass that information, so that we can build on top > of that abstraction that is *not* pure POSIX abstraction, no? After > all, you are the one who constantly whine that Git is implemented on > POSIX API and it is inconvenient for other platforms. There is another issue with the existing code that this new "xpoll" will need to take into account. If a SIGCHLD arrives between the call to check_dead_children and poll, the poll will not be interupted by it, resulting in the child not being reaped until another child terminates or a client connects. Currently, the effect is just a zombie process for a longer time, however, the proposed patch (daemon: graceful shutdown of client connection) relies on the cleanup to close the client connection. When I have time, I will reroll including a change to ppoll. -Kim
Re: [PATCH v10 25/36] merge-recursive: fix overwriting dirty files involved in renames
On Thu, Apr 19, 2018 at 1:48 PM, Martin Ågren wrote: > On 19 April 2018 at 19:58, Elijah Newren wrote: >> This fixes an issue that existed before my directory rename detection >> patches that affects both normal renames and renames implied by >> directory rename detection. Additional codepaths that only affect >> overwriting of dirty files that are involved in directory rename >> detection will be added in a subsequent commit. >> >> Reviewed-by: Stefan Beller >> Signed-off-by: Elijah Newren >> Signed-off-by: Junio C Hamano >> --- >> merge-recursive.c | 85 ++--- >> merge-recursive.h | 2 + >> t/t3501-revert-cherry-pick.sh | 2 +- >> t/t6043-merge-rename-directories.sh | 2 +- >> t/t7607-merge-overwrite.sh | 2 +- >> unpack-trees.c | 4 +- >> unpack-trees.h | 4 ++ >> 7 files changed, 77 insertions(+), 24 deletions(-) >> >> diff --git a/merge-recursive.c b/merge-recursive.c >> index c1c4faf61e..7fdcba4f22 100644 >> --- a/merge-recursive.c >> +++ b/merge-recursive.c >> @@ -337,32 +337,37 @@ static void init_tree_desc_from_tree(struct tree_desc >> *desc, struct tree *tree) >> init_tree_desc(desc, tree->buffer, tree->size); >> } >> >> -static int git_merge_trees(int index_only, >> +static int git_merge_trees(struct merge_options *o, >>struct tree *common, >>struct tree *head, >>struct tree *merge) >> { >> int rc; >> struct tree_desc t[3]; >> - struct unpack_trees_options opts; >> >> - memset(&opts, 0, sizeof(opts)); >> - if (index_only) >> - opts.index_only = 1; >> + memset(&o->unpack_opts, 0, sizeof(o->unpack_opts)); >> + if (o->call_depth) >> + o->unpack_opts.index_only = 1; >> else >> - opts.update = 1; >> - opts.merge = 1; >> - opts.head_idx = 2; >> - opts.fn = threeway_merge; >> - opts.src_index = &the_index; >> - opts.dst_index = &the_index; >> - setup_unpack_trees_porcelain(&opts, "merge"); >> + o->unpack_opts.update = 1; >> + o->unpack_opts.merge = 1; >> + o->unpack_opts.head_idx = 2; >> + o->unpack_opts.fn = threeway_merge; >> + o->unpack_opts.src_index = &the_index; >> + o->unpack_opts.dst_index = &the_index; >> + setup_unpack_trees_porcelain(&o->unpack_opts, "merge"); >> >> init_tree_desc_from_tree(t+0, common); >> init_tree_desc_from_tree(t+1, head); >> init_tree_desc_from_tree(t+2, merge); >> >> - rc = unpack_trees(3, t, &opts); >> + rc = unpack_trees(3, t, &o->unpack_opts); >> + /* >> +* unpack_trees NULLifies src_index, but it's used in >> verify_uptodate, >> +* so set to the new index which will usually have modification >> +* timestamp info copied over. >> +*/ >> + o->unpack_opts.src_index = &the_index; >> cache_tree_free(&active_cache_tree); >> return rc; >> } > > As mentioned in a reply to patch 33/36 [1], I've got a patch to add > `clear_unpack_trees_porcelain()` which frees the resources allocated by > `setup_unpack_trees_porcelain()`. Before this patch, I could easily call > it at the end of this function. After this, the ownership is less > obvious to me. I wouldn't put the call to clear_unpack_trees_porcelain() at the end of this function, but rather at the end of merge_trees(). merge_trees() is the only caller of git_merge_trees() and it continues using o->unpack_opts until the end of that function. At the end of that function, there is no further need for o->unpack_opts. Basically, put it right where I put the "FIXME: Need to also free data allocated by setup_unpack_trees_porcelain()" comment.
Re: [PATCH v10 25/36] merge-recursive: fix overwriting dirty files involved in renames
On 19 April 2018 at 22:48, Martin Ågren wrote: > On 19 April 2018 at 19:58, Elijah Newren wrote: >> -static int git_merge_trees(int index_only, >> +static int git_merge_trees(struct merge_options *o, >>struct tree *common, >>struct tree *head, >>struct tree *merge) >> { [...] >> + memset(&o->unpack_opts, 0, sizeof(o->unpack_opts)); [...] >> + setup_unpack_trees_porcelain(&o->unpack_opts, "merge"); [...] >> } > > As mentioned in a reply to patch 33/36 [1], I've got a patch to add > `clear_unpack_trees_porcelain()` which frees the resources allocated by > `setup_unpack_trees_porcelain()`. Before this patch, I could easily call > it at the end of this function. After this, the ownership is less > obvious to me. > > It turns out that the only user of `unpack_opts` outside this function > can indeed end up wanting to use the error messages that `clear_...()` > would set out to free. So yes, the call to `clear_...()` will need to go > elsewhere. > > It does sort of make me wonder if we should memset `unpack_opts` to zero > somewhere early, so that we can then `clear_...()` it early here before > zeroizing it. So yes, we'd be constantly allocating and freeing those > strings. Am I right to assume that the code after your series would do > (roughly) the same number of calls to `setup_unpack_trees_porcelain()`, > i.e., `git_merge_trees()` as it did before? Or, of course, both `setup_...` and `clear_...` would go outside this function to churn less memory... Anyway, this still holds: > All of this is arguably irrelevant for this series. It might be better > if I clarify this memory ownership and do any adjustments as part of my > patch (series), rather than you shuffling things around at this time.
Re: [PATCH v10 32/36] t6046: testcases checking whether updates can be skipped in a merge
On Thu, Apr 19, 2018 at 1:26 PM, SZEDER Gábor wrote: > Just a couple of minor things: Sweet, thanks for taking a look; will get these all fixed up.
Re: [PATCH v10 33/36] merge-recursive: fix was_tracked() to quit lying with some renamed paths
On Thu, Apr 19, 2018 at 1:39 PM, Martin Ågren wrote: > On 19 April 2018 at 19:58, Elijah Newren wrote: >> + /* Free the extra index left from git_merge_trees() */ >> + /* >> +* FIXME: Need to also data allocated by >> setup_unpack_trees_porcelain() >> +* tucked away in o->unpack_opts.msgs, but the problem is that only >> +* half of it refers to dynamically allocated data, while the other >> +* half points at static strings. >> +*/ > > Timing. I've been preparing a patch that provides > `clear_unpack_trees_porcelain()` and fixes all such leaks. (About 10% of > all the leaks that are reported when I run the test-suite!) My patch Nice! > conflicts with this series for obvious reasons. Figuring out the > conflict resolution might be non-trivial, and I suspect it would even be > an evil merge. I'll be holding off on that patch until this has landed. > > BTW: s/also data/also free data/. But since I'm promising to get rid of > this TODO quite soon after this is merged... ;-) Oops, good catch. I can fix it up since I need to fix the issues SZEDER found, but yeah if you're just going to implement the fix and rip this comment out then it's not that critical.
Re: [PATCH v10 25/36] merge-recursive: fix overwriting dirty files involved in renames
On 19 April 2018 at 19:58, Elijah Newren wrote: > This fixes an issue that existed before my directory rename detection > patches that affects both normal renames and renames implied by > directory rename detection. Additional codepaths that only affect > overwriting of dirty files that are involved in directory rename > detection will be added in a subsequent commit. > > Reviewed-by: Stefan Beller > Signed-off-by: Elijah Newren > Signed-off-by: Junio C Hamano > --- > merge-recursive.c | 85 ++--- > merge-recursive.h | 2 + > t/t3501-revert-cherry-pick.sh | 2 +- > t/t6043-merge-rename-directories.sh | 2 +- > t/t7607-merge-overwrite.sh | 2 +- > unpack-trees.c | 4 +- > unpack-trees.h | 4 ++ > 7 files changed, 77 insertions(+), 24 deletions(-) > > diff --git a/merge-recursive.c b/merge-recursive.c > index c1c4faf61e..7fdcba4f22 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -337,32 +337,37 @@ static void init_tree_desc_from_tree(struct tree_desc > *desc, struct tree *tree) > init_tree_desc(desc, tree->buffer, tree->size); > } > > -static int git_merge_trees(int index_only, > +static int git_merge_trees(struct merge_options *o, >struct tree *common, >struct tree *head, >struct tree *merge) > { > int rc; > struct tree_desc t[3]; > - struct unpack_trees_options opts; > > - memset(&opts, 0, sizeof(opts)); > - if (index_only) > - opts.index_only = 1; > + memset(&o->unpack_opts, 0, sizeof(o->unpack_opts)); > + if (o->call_depth) > + o->unpack_opts.index_only = 1; > else > - opts.update = 1; > - opts.merge = 1; > - opts.head_idx = 2; > - opts.fn = threeway_merge; > - opts.src_index = &the_index; > - opts.dst_index = &the_index; > - setup_unpack_trees_porcelain(&opts, "merge"); > + o->unpack_opts.update = 1; > + o->unpack_opts.merge = 1; > + o->unpack_opts.head_idx = 2; > + o->unpack_opts.fn = threeway_merge; > + o->unpack_opts.src_index = &the_index; > + o->unpack_opts.dst_index = &the_index; > + setup_unpack_trees_porcelain(&o->unpack_opts, "merge"); > > init_tree_desc_from_tree(t+0, common); > init_tree_desc_from_tree(t+1, head); > init_tree_desc_from_tree(t+2, merge); > > - rc = unpack_trees(3, t, &opts); > + rc = unpack_trees(3, t, &o->unpack_opts); > + /* > +* unpack_trees NULLifies src_index, but it's used in verify_uptodate, > +* so set to the new index which will usually have modification > +* timestamp info copied over. > +*/ > + o->unpack_opts.src_index = &the_index; > cache_tree_free(&active_cache_tree); > return rc; > } As mentioned in a reply to patch 33/36 [1], I've got a patch to add `clear_unpack_trees_porcelain()` which frees the resources allocated by `setup_unpack_trees_porcelain()`. Before this patch, I could easily call it at the end of this function. After this, the ownership is less obvious to me. It turns out that the only user of `unpack_opts` outside this function can indeed end up wanting to use the error messages that `clear_...()` would set out to free. So yes, the call to `clear_...()` will need to go elsewhere. It does sort of make me wonder if we should memset `unpack_opts` to zero somewhere early, so that we can then `clear_...()` it early here before zeroizing it. So yes, we'd be constantly allocating and freeing those strings. Am I right to assume that the code after your series would do (roughly) the same number of calls to `setup_unpack_trees_porcelain()`, i.e., `git_merge_trees()` as it did before? All of this is arguably irrelevant for this series. It might be better if I clarify this memory ownership and do any adjustments as part of my patch (series), rather than you shuffling things around at this time. Mostly thinking out loud. If you have any thoughts, feel free to share. Martin [1] https://public-inbox.org/git/can0hesqujbommgay+5xomqxcgohtxxf1mjbmy_l7y+aa4eg...@mail.gmail.com/
Re: [PATCH v10 33/36] merge-recursive: fix was_tracked() to quit lying with some renamed paths
On 19 April 2018 at 19:58, Elijah Newren wrote: > + /* Free the extra index left from git_merge_trees() */ > + /* > +* FIXME: Need to also data allocated by > setup_unpack_trees_porcelain() > +* tucked away in o->unpack_opts.msgs, but the problem is that only > +* half of it refers to dynamically allocated data, while the other > +* half points at static strings. > +*/ Timing. I've been preparing a patch that provides `clear_unpack_trees_porcelain()` and fixes all such leaks. (About 10% of all the leaks that are reported when I run the test-suite!) My patch conflicts with this series for obvious reasons. Figuring out the conflict resolution might be non-trivial, and I suspect it would even be an evil merge. I'll be holding off on that patch until this has landed. BTW: s/also data/also free data/. But since I'm promising to get rid of this TODO quite soon after this is merged... ;-) Martin
Re: [PATCH v10 32/36] t6046: testcases checking whether updates can be skipped in a merge
Just a couple of minor things: > +### > +# SECTION 1: Cases involving no renames (one side has subset of changes of > +#the other side) > +### > + > +# Testcase 1a, Changes on A, subset of changes on B > +# Commit O: b_1 > +# Commit A: b_2 > +# Commit B: b_3 > +# Expected: b_2 > + > +test_expect_success '1a-setup: Modify(A)/Modify(B), change on B subset of A' > ' > + test_create_repo 1a && > + ( > + cd 1a && > + > + test_write_lines 1 2 3 4 5 6 7 8 9 10 >b Broken && chain. <...> > +### > +# SECTION 2: Cases involving basic renames > +### > + > +# Testcase 2a, Changes on A, rename on B > +# Commit O: b_1 > +# Commit A: b_2 > +# Commit B: c_1 > +# Expected: c_2 > + > +test_expect_success '2a-setup: Modify(A)/rename(B)' ' > + test_create_repo 2a && > + ( > + cd 2a && > + > + test_seq 1 10 >b Broken && chain. > + git add b && > + test_tick && > + git commit -m "O" && > + > + git branch O && > + git branch A && > + git branch B && > + > + git checkout A && > + test_seq 1 11 > b && Nit: space between redirection operator and filename. <...> > +# Testcase 2b, Changed and renamed on A, subset of changes on B > +# Commit O: b_1 > +# Commit A: c_2 > +# Commit B: b_3 > +# Expected: c_2 > + > +test_expect_success '2b-setup: Rename+Mod(A)/Mod(B), B mods subset of A' ' > + test_create_repo 2b && > + ( > + cd 2b && > + > + test_write_lines 1 2 3 4 5 6 7 8 9 10 >b Broken && chain. <...> > +# Testcase 2c, Changes on A, rename on B > +# Commit O: b_1 > +# Commit A: b_2, c_3 > +# Commit B: c_1 > +# Expected: rename/add conflict c_2 vs c_3 > +# > +# NOTE: Since A modified b_1->b_2, and B renamed b_1->c_1, the threeway > +# merge of those files should result in c_2. We then should have a > +# rename/add conflict between c_2 and c_3. However, if we note in > +# merge_content() that A had the right contents (b_2 has same > +# contents as c_2, just at a different name), and that A had the > +# right path present (c_3 existed) and thus decides that it can > +# skip the update, then we're in trouble. This test verifies we do > +# not make that particular mistake. > + > +test_expect_success '2c-setup: Modify b & add c VS rename b->c' ' > + test_create_repo 2c && > + ( > + cd 2c && > + > + test_seq 1 10 >b Broken && chain. <...> > +### > +# SECTION 3: Cases involving directory renames > +# > +# NOTE: > +# Directory renames only apply when one side renames a directory, and the > +# other side adds or renames a path into that directory. Applying the > +# directory rename to that new path creates a new pathname that didn't > +# exist on either side of history. Thus, it is impossible for the > +# merge contents to already be at the right path, so all of these checks > +# exist just to make sure that updates are not skipped. > +### > + > +# Testcase 3a, Change + rename into dir foo on A, dir rename foo->bar on B > +# Commit O: bq_1, foo/whatever > +# Commit A: foo/{bq_2, whatever} > +# Commit B: bq_1, bar/whatever > +# Expected: bar/{bq_2, whatever} > + > +test_expect_success '3a-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' ' > + test_create_repo 3a && > + ( > + cd 3a && > + > + mkdir foo && > + test_seq 1 10 >bq && > + test_write_lines a b c d e f g h i j k >foo/whatever && > + git add bq foo/whatever && > + test_tick && > + git commit -m "O" && > + > + git branch O && > + git branch A && > + git branch B && > + > + git checkout A && > + test_seq 1 11 > bq && Space between redirection operator and filename. <...> > +# Testcase 3b, rename into dir foo on A, dir rename foo->bar + change on B > +# Commit O: bq_1, foo/whatever > +# Commit A: foo/{bq_1, whatever} > +# Commit B: bq_2, bar/whatever > +# Expected: bar/{bq_2, whatever} > + > +test_expect_success '3b-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' ' > + test_create_repo 3b && > + ( > + cd 3b && > + > + mkdir foo && > + test_seq 1 10 >bq && > + test_write_lines a b c d e f g h i j k >foo/whatever && > + git add bq foo/whatever && > + test_tick && > +
Re: [PATCH v10 00/36] Add directory rename detection to git
On Thu, Apr 19, 2018 at 11:35 AM, Elijah Newren wrote: > On Thu, Apr 19, 2018 at 10:57 AM, Elijah Newren wrote: >> This series is a reboot of the directory rename detection series that was >> merged to master and then reverted due to the final patch having a buggy >> can-skip-update check, as noted at >> https://public-inbox.org/git/xmqqmuya43cs@gitster-ct.c.googlers.com/ >> This series based on top of master. > > ...and merges cleanly to next but apparently has some minor conflicts > with both ds/lazy-load-trees and ps/test-chmtime-get from pu. > > What's the preferred way to resolve this? Rebase and resubmit my > series on pu, or something else? Sorry, user error; there are no conflicts with my series. (I accidentally included Junio's interim round of my own series and while trying to spot problems I saw commits from these other series touching relevant files in what looked like nearby areas. Directly merging with these other two series or even merging all of pu before en/rename-directory-detection-reboot followed by individually merging later series has no conflicts with any of my changes.)
Re: [PATCH v10 00/36] Add directory rename detection to git
On 4/19/2018 2:41 PM, Stefan Beller wrote: On Thu, Apr 19, 2018 at 11:35 AM, Elijah Newren wrote: On Thu, Apr 19, 2018 at 10:57 AM, Elijah Newren wrote: This series is a reboot of the directory rename detection series that was merged to master and then reverted due to the final patch having a buggy can-skip-update check, as noted at https://public-inbox.org/git/xmqqmuya43cs@gitster-ct.c.googlers.com/ This series based on top of master. ...and merges cleanly to next but apparently has some minor conflicts with both ds/lazy-load-trees and ps/test-chmtime-get from pu. What's the preferred way to resolve this? Rebase and resubmit my series on pu, or something else? If you were to base it off of pu, this series would depend on all other series that pu contains. This is bad for the progress of this series. (If it were to be merged to next, all other series would automatically merge to next as well) If the conflicts are minor, then Junio resolves them; if you want to be nice, pick your merge point as git checkout origin/master git merge ds/lazy-load-trees git merge ps/test-chmtime-get git tag my-anchor and put the series on top of that anchor. If you do this, you'd want to be reasonably sure that those two series are not in too much flux. I believe ds/lazy-load-trees is queued for 'next'. I'm not surprised that there are some conflicts here. Any reference to the 'tree' member of a commit should be replaced with 'get_commit_tree(c)', or 'get_commit_tree_oid(c)' if you only care about the tree's object id. I think Stefan's suggestion is the best approach to get the right conflicts out of the way. Thanks, -Stolee
Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames
On Wed, Apr 18, 2018 at 2:31 PM, Johannes Schindelin wrote: > I suspect that the culprit is once again Cygwin's trick where illegal > characters are mapped into a private Unicode page. Cygwin (and therefore > MSYS2 runtime, and therefore the Bash used to run the test script) can use > those filenames all right, but Git cannot. > > So even testing whether you could write an illegal file name via shell > script is *not* enough to determine whether the file system supports funny > characters. I followed suit of all existing FUNNYNAMES checks: $ git grep -B3 'test_set_prereq FUNNYNAMES' master t/ master:t/t3600-rm.sh-if test_have_prereq !MINGW && touch -- 'tab embedded' 'newline master:t/t3600-rm.sh-embedded' 2>/dev/null master:t/t3600-rm.sh-then master:t/t3600-rm.sh: test_set_prereq FUNNYNAMES -- master:t/t4135-apply-weird-filenames.sh-if test_have_prereq !MINGW && master:t/t4135-apply-weird-filenames.sh-touch -- "tab embedded.txt" '\''"quoteembedded".txt'\'' master:t/t4135-apply-weird-filenames.sh-then master:t/t4135-apply-weird-filenames.sh: test_set_prereq FUNNYNAMES -- master:t/t9903-bash-prompt.sh- master:t/t9903-bash-prompt.sh-if test_have_prereq !MINGW && mkdir "$repo_with_newline" 2>/dev/null master:t/t9903-bash-prompt.sh-then master:t/t9903-bash-prompt.sh: test_set_prereq FUNNYNAMES How am I supposed to check this, then? > As far as I can tell from a *really* cursory glance, this is the only > affected test case. Apparently your prereq catches, somehow, on Windows: > > 2018-04-18T11:12:43.0459702Z Your filesystem does not allow \ and " in > filenames. > 2018-04-18T11:12:43.0459823Z skipped: complete files - C-style escapes in > ls-files output (missing FUNNYNAMES_BS_DQ)
Re: [PATCH v2 1/1] completion: load completion file for external subcommand
On 2018-04-18 21:51, SZEDER Gábor wrote: On Tue, Apr 10, 2018 at 10:28 PM, Florian Gamböck wrote: Adding external subcommands to Git is as easy as to put an executable file git-foo into PATH. Packaging such subcommands for a Linux distribution can be achieved by unpacking the executable into /usr/bin of the user's system. Adding system-wide completion scripts for new subcommands, however, can be a bit tricky. Since bash-completion started to use dynamical loading of completion scripts since v1.90 (preview of v2.0), I believe the main bash-completion repository can be found at: https://github.com/scop/bash-completion.git This repository still contains the branch 'dynamic-loading'; for the record it points to 3b029892f6f9db3b7210a7f66d636be3e5ec5fa2. Two commits on that branch are worth mentioning: 20c05b43 (Load completions in separate files dynamically, get rid of have()., 2011-10-12) 5baebf81 (Add _xfunc for loading and calling functions on demand, use it in apt-get, cvsps, rsync, and sshfs., 2011-10-13) Nice, thanks for the pointers! (...) I think the easiest method is to use a function that is defined by bash-completion v2.0+, namely __load_completion. This is wrong, __load_completion() was introduced in cad3abfc (__load_completion: New function, use in _completion_loader and _xfunc, 2015-07-15), and the first release tag containg it is '2.2' from 2016-03-03. Dang, I thought it was introduced at the same time. Sorry for that. I guess, 2016 is a bit too young to take it for granted then? The release tags '1.90' and '2.0' are from 2011-11-03 and 2012-06-17, respectively. This leaves a couple of years long hole where completions were already loaded dynamically but there was no __load_completion() function. Would it be possible to use _xfunc() instead to plug that hole? It seems the be tricky, because that function not only sources but also _calls_ the completion function. But isn't this exactly what we want? Lucky us, we can replace the whole if-fi block with a simpler: _xfunc git-$command $completion_func 2>/dev/null && return If _xfunc is not defined -- as in, bashcomp is not installed / loaded -- then the return will not get called and the original completion will continue: declare -f $completion_func >/dev/null 2>/dev/null && $completion_func && return Since this would be redundant, we could define a fall-back for _xfunc like so: declare -f _xfunc || _xfunc() { declare -f $completion_func >/dev/null 2>/dev/null && $completion_func && return } This way, we retain the "old" behavior and get dynamic loading if bashcomp is available. The actual call to get the completions would just be _xfunc like in my first example above. What do you think? -- Regards Florian
Re: [PATCH v10 00/36] Add directory rename detection to git
On Thu, Apr 19, 2018 at 11:35 AM, Elijah Newren wrote: > On Thu, Apr 19, 2018 at 10:57 AM, Elijah Newren wrote: >> This series is a reboot of the directory rename detection series that was >> merged to master and then reverted due to the final patch having a buggy >> can-skip-update check, as noted at >> https://public-inbox.org/git/xmqqmuya43cs@gitster-ct.c.googlers.com/ >> This series based on top of master. > > ...and merges cleanly to next but apparently has some minor conflicts > with both ds/lazy-load-trees and ps/test-chmtime-get from pu. > > What's the preferred way to resolve this? Rebase and resubmit my > series on pu, or something else? If you were to base it off of pu, this series would depend on all other series that pu contains. This is bad for the progress of this series. (If it were to be merged to next, all other series would automatically merge to next as well) If the conflicts are minor, then Junio resolves them; if you want to be nice, pick your merge point as git checkout origin/master git merge ds/lazy-load-trees git merge ps/test-chmtime-get git tag my-anchor and put the series on top of that anchor. If you do this, you'd want to be reasonably sure that those two series are not in too much flux. Thanks, Stefan
Re: [PATCH v10 00/36] Add directory rename detection to git
On Thu, Apr 19, 2018 at 10:57 AM, Elijah Newren wrote: > This series is a reboot of the directory rename detection series that was > merged to master and then reverted due to the final patch having a buggy > can-skip-update check, as noted at > https://public-inbox.org/git/xmqqmuya43cs@gitster-ct.c.googlers.com/ > This series based on top of master. ...and merges cleanly to next but apparently has some minor conflicts with both ds/lazy-load-trees and ps/test-chmtime-get from pu. What's the preferred way to resolve this? Rebase and resubmit my series on pu, or something else?
[PATCH v10 12/36] merge-recursive: move the get_renames() function
Move this function so it can re-use some others (without either moving all of them or adding an annoying split between function declarations and definitions). Cheat slightly by adding a blank line for readability, and in order to silence checkpatch.pl. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 139 +++--- 1 file changed, 70 insertions(+), 69 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 0c0d48624d..973b6e2985 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -537,75 +537,6 @@ struct rename { unsigned processed:1; }; -/* - * Get information of all renames which occurred between 'o_tree' and - * 'tree'. We need the three trees in the merge ('o_tree', 'a_tree' and - * 'b_tree') to be able to associate the correct cache entries with - * the rename information. 'tree' is always equal to either a_tree or b_tree. - */ -static struct string_list *get_renames(struct merge_options *o, - struct tree *tree, - struct tree *o_tree, - struct tree *a_tree, - struct tree *b_tree, - struct string_list *entries) -{ - int i; - struct string_list *renames; - struct diff_options opts; - - renames = xcalloc(1, sizeof(struct string_list)); - if (!o->detect_rename) - return renames; - - diff_setup(&opts); - opts.flags.recursive = 1; - opts.flags.rename_empty = 0; - opts.detect_rename = DIFF_DETECT_RENAME; - opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit : - o->diff_rename_limit >= 0 ? o->diff_rename_limit : - 1000; - opts.rename_score = o->rename_score; - opts.show_rename_progress = o->show_rename_progress; - opts.output_format = DIFF_FORMAT_NO_OUTPUT; - diff_setup_done(&opts); - diff_tree_oid(&o_tree->object.oid, &tree->object.oid, "", &opts); - diffcore_std(&opts); - if (opts.needed_rename_limit > o->needed_rename_limit) - o->needed_rename_limit = opts.needed_rename_limit; - for (i = 0; i < diff_queued_diff.nr; ++i) { - struct string_list_item *item; - struct rename *re; - struct diff_filepair *pair = diff_queued_diff.queue[i]; - if (pair->status != 'R') { - diff_free_filepair(pair); - continue; - } - re = xmalloc(sizeof(*re)); - re->processed = 0; - re->pair = pair; - item = string_list_lookup(entries, re->pair->one->path); - if (!item) - re->src_entry = insert_stage_data(re->pair->one->path, - o_tree, a_tree, b_tree, entries); - else - re->src_entry = item->util; - - item = string_list_lookup(entries, re->pair->two->path); - if (!item) - re->dst_entry = insert_stage_data(re->pair->two->path, - o_tree, a_tree, b_tree, entries); - else - re->dst_entry = item->util; - item = string_list_insert(renames, pair->one->path); - item->util = re; - } - opts.output_format = DIFF_FORMAT_NO_OUTPUT; - diff_queued_diff.nr = 0; - diff_flush(&opts); - return renames; -} - static int update_stages(struct merge_options *opt, const char *path, const struct diff_filespec *o, const struct diff_filespec *a, @@ -1390,6 +1321,76 @@ static int conflict_rename_rename_2to1(struct merge_options *o, return ret; } +/* + * Get information of all renames which occurred between 'o_tree' and + * 'tree'. We need the three trees in the merge ('o_tree', 'a_tree' and + * 'b_tree') to be able to associate the correct cache entries with + * the rename information. 'tree' is always equal to either a_tree or b_tree. + */ +static struct string_list *get_renames(struct merge_options *o, + struct tree *tree, + struct tree *o_tree, + struct tree *a_tree, + struct tree *b_tree, + struct string_list *entries) +{ + int i; + struct string_list *renames; + struct diff_options opts; + + renames = xcalloc(1, sizeof(struct string_list)); + if (!o->detect_rename) + return renames; + + diff_setup(&opts); + opts.flags.recursive = 1; + opts.flags.rename_empty = 0; + opts.detect_
[PATCH v10 05/36] directory rename detection: files/directories in the way of some renames
Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6043-merge-rename-directories.sh | 330 1 file changed, 330 insertions(+) diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index 713ad2b75e..b469c807c2 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -850,4 +850,334 @@ test_expect_success '4a-check: Directory split, with original directory still pr # detection.) But, sadly, see testcase 8b. ### + +### +# SECTION 5: Files/directories in the way of subset of to-be-renamed paths +# +# Implicitly renaming files due to a detected directory rename could run +# into problems if there are files or directories in the way of the paths +# we want to rename. Explore such cases in this section. +### + +# Testcase 5a, Merge directories, other side adds files to original and target +# Commit O: z/{b,c}, y/d +# Commit A: z/{b,c,e_1,f}, y/{d,e_2} +# Commit B: y/{b,c,d} +# Expected: z/e_1, y/{b,c,d,e_2,f} + CONFLICT warning +# NOTE: While directory rename detection is active here causing z/f to +# become y/f, we did not apply this for z/e_1 because that would +# give us an add/add conflict for y/e_1 vs y/e_2. This problem with +# this add/add, is that both versions of y/e are from the same side +# of history, giving us no way to represent this conflict in the +# index. + +test_expect_success '5a-setup: Merge directories, other side adds files to original and target' ' + test_create_repo 5a && + ( + cd 5a && + + mkdir z && + echo b >z/b && + echo c >z/c && + mkdir y && + echo d >y/d && + git add z y && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + echo e1 >z/e && + echo f >z/f && + echo e2 >y/e && + git add z/e z/f y/e && + test_tick && + git commit -m "A" && + + git checkout B && + git mv z/b y/ && + git mv z/c y/ && + rmdir z && + test_tick && + git commit -m "B" + ) +' + +test_expect_failure '5a-check: Merge directories, other side adds files to original and target' ' + ( + cd 5a && + + git checkout A^0 && + + test_must_fail git merge -s recursive B^0 >out && + test_i18ngrep "CONFLICT.*implicit dir rename" out && + + git ls-files -s >out && + test_line_count = 6 out && + git ls-files -u >out && + test_line_count = 0 out && + git ls-files -o >out && + test_line_count = 1 out && + + git rev-parse >actual \ + :0:y/b :0:y/c :0:y/d :0:y/e :0:z/e :0:y/f && + git rev-parse >expect \ +O:z/b O:z/c O:y/d A:y/e A:z/e A:z/f && + test_cmp expect actual + ) +' + +# Testcase 5b, Rename/delete in order to get add/add/add conflict +# (Related to testcase 8d; these may appear slightly inconsistent to users; +#Also related to testcases 7d and 7e) +# Commit O: z/{b,c,d_1} +# Commit A: y/{b,c,d_2} +# Commit B: z/{b,c,d_1,e}, y/d_3 +# Expected: y/{b,c,e}, CONFLICT(add/add: y/d_2 vs. y/d_3) +# NOTE: If z/d_1 in commit B were to be involved in dir rename detection, as +# we normaly would since z/ is being renamed to y/, then this would be +# a rename/delete (z/d_1 -> y/d_1 vs. deleted) AND an add/add/add +# conflict of y/d_1 vs. y/d_2 vs. y/d_3. Add/add/add is not +# representable in the index, so the existence of y/d_3 needs to +# cause us to bail on directory rename detection for that path, falling +# back to git behavior without the directory rename detection. + +test_expect_success '5b-setup: Rename/delete in order to get add/add/add conflict' ' + test_create_repo 5b && + ( + cd 5b && + + mkdir z && + echo b >z/b && + echo c >z/c && + echo d1 >z/d && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + git rm z/d && + git mv z y && + echo d2 >y/d && + git add
[PATCH v10 03/36] directory rename detection: testcases to avoid taking detection too far
Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6043-merge-rename-directories.sh | 153 1 file changed, 153 insertions(+) diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index b22a9052b3..8049ed5fc9 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -582,4 +582,157 @@ test_expect_success '2b-check: Directory split into two on one side, with equal # messages are handled correctly. ### + +### +# SECTION 3: Path in question is the source path for some rename already +# +# Combining cases from Section 1 and trying to handle them could lead to +# directory renaming detection being over-applied. So, this section +# provides some good testcases to check that the implementation doesn't go +# too far. +### + +# Testcase 3a, Avoid implicit rename if involved as source on other side +# (Related to testcases 1c and 1f) +# Commit O: z/{b,c,d} +# Commit A: z/{b,c,d} (no change) +# Commit B: y/{b,c}, x/d +# Expected: y/{b,c}, x/d +test_expect_success '3a-setup: Avoid implicit rename if involved as source on other side' ' + test_create_repo 3a && + ( + cd 3a && + + mkdir z && + echo b >z/b && + echo c >z/c && + echo d >z/d && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + test_tick && + git commit --allow-empty -m "A" && + + git checkout B && + mkdir y && + mkdir x && + git mv z/b y/ && + git mv z/c y/ && + git mv z/d x/ && + rmdir z && + test_tick && + git commit -m "B" + ) +' + +test_expect_success '3a-check: Avoid implicit rename if involved as source on other side' ' + ( + cd 3a && + + git checkout A^0 && + + git merge -s recursive B^0 && + + git ls-files -s >out && + test_line_count = 3 out && + + git rev-parse >actual \ + HEAD:y/b HEAD:y/c HEAD:x/d && + git rev-parse >expect \ + O:z/bO:z/cO:z/d && + test_cmp expect actual + ) +' + +# Testcase 3b, Avoid implicit rename if involved as source on other side +# (Related to testcases 5c and 7c, also kind of 1e and 1f) +# Commit O: z/{b,c,d} +# Commit A: y/{b,c}, x/d +# Commit B: z/{b,c}, w/d +# Expected: y/{b,c}, CONFLICT:(z/d -> x/d vs. w/d) +# NOTE: We're particularly checking that since z/d is already involved as +# a source in a file rename on the same side of history, that we don't +# get it involved in directory rename detection. If it were, we might +# end up with CONFLICT:(z/d -> y/d vs. x/d vs. w/d), i.e. a +# rename/rename/rename(1to3) conflict, which is just weird. +test_expect_success '3b-setup: Avoid implicit rename if involved as source on current side' ' + test_create_repo 3b && + ( + cd 3b && + + mkdir z && + echo b >z/b && + echo c >z/c && + echo d >z/d && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + mkdir y && + mkdir x && + git mv z/b y/ && + git mv z/c y/ && + git mv z/d x/ && + rmdir z && + test_tick && + git commit -m "A" && + + git checkout B && + mkdir w && + git mv z/d w/ && + test_tick && + git commit -m "B" + ) +' + +test_expect_success '3b-check: Avoid implicit rename if involved as source on current side' ' + ( + cd 3b && + + git checkout A^0 && + + test_must_fail git merge -s recursive B^0 >out && + test_i18ngrep CONFLICT.*rename/rename.*z/d.*x/d.*w/d out && + test_i18ngrep ! CONFLICT.*rename/rename.*y/d out && + + git ls-files -s >out && + test_line_count = 5 out && + git ls-files -u >out && + test_line_count = 3 out && + git ls-files -o >out && + test_line_count = 1 out && + + git rev-parse >actu
[PATCH v10 11/36] directory rename detection: tests for handling overwriting dirty files
Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6043-merge-rename-directories.sh | 458 1 file changed, 458 insertions(+) diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index a6cd38336c..8ea9ec49bc 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -3246,4 +3246,462 @@ test_expect_failure '10e-check: Does git complain about untracked file that is n ) ' +### +# SECTION 11: Handling dirty (not up-to-date) files +# +# unpack_trees(), upon which the recursive merge algorithm is based, aborts +# the operation if untracked or dirty files would be deleted or overwritten +# by the merge. Unfortunately, unpack_trees() does not understand renames, +# and if it doesn't abort, then it muddies up the working directory before +# we even get to the point of detecting renames, so we need some special +# handling. This was true even of normal renames, but there are additional +# codepaths that need special handling with directory renames. Add +# testcases for both renamed-by-directory-rename-detection and standard +# rename cases. +### + +# Testcase 11a, Avoid losing dirty contents with simple rename +# Commit O: z/{a,b_v1}, +# Commit A: z/{a,c_v1}, and z/c_v1 has uncommitted mods +# Commit B: z/{a,b_v2} +# Expected: ERROR_MSG(Refusing to lose dirty file at z/c) + +# z/a, staged version of z/c has sha1sum matching B:z/b_v2, +# z/c~HEAD with contents of B:z/b_v2, +# z/c with uncommitted mods on top of A:z/c_v1 + +test_expect_success '11a-setup: Avoid losing dirty contents with simple rename' ' + test_create_repo 11a && + ( + cd 11a && + + mkdir z && + echo a >z/a && + test_seq 1 10 >z/b && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + git mv z/b z/c && + test_tick && + git commit -m "A" && + + git checkout B && + echo 11 >>z/b && + git add z/b && + test_tick && + git commit -m "B" + ) +' + +test_expect_failure '11a-check: Avoid losing dirty contents with simple rename' ' + ( + cd 11a && + + git checkout A^0 && + echo stuff >>z/c && + + test_must_fail git merge -s recursive B^0 >out 2>err && + test_i18ngrep "Refusing to lose dirty file at z/c" out && + + test_seq 1 10 >expected && + echo stuff >>expected && + test_cmp expected z/c && + + git ls-files -s >out && + test_line_count = 2 out && + git ls-files -u >out && + test_line_count = 1 out && + git ls-files -o >out && + test_line_count = 4 out && + + git rev-parse >actual \ + :0:z/a :2:z/c && + git rev-parse >expect \ +O:z/a B:z/b && + test_cmp expect actual && + + git hash-object z/c~HEAD >actual && + git rev-parse B:z/b >expect && + test_cmp expect actual + ) +' + +# Testcase 11b, Avoid losing dirty file involved in directory rename +# Commit O: z/a, x/{b,c_v1} +# Commit A: z/{a,c_v1}, x/b, and z/c_v1 has uncommitted mods +# Commit B: y/a, x/{b,c_v2} +# Expected: y/{a,c_v2}, x/b, z/c_v1 with uncommitted mods untracked, +# ERROR_MSG(Refusing to lose dirty file at z/c) + + +test_expect_success '11b-setup: Avoid losing dirty file involved in directory rename' ' + test_create_repo 11b && + ( + cd 11b && + + mkdir z x && + echo a >z/a && + echo b >x/b && + test_seq 1 10 >x/c && + git add z x && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + git mv x/c z/c && + test_tick && + git commit -m "A" && + + git checkout B && + git mv z y && + echo 11 >>x/c && + git add x/c && + test_tick && + git commit -m "B" + ) +' + +test_expect_failure '11b-check: Avoid losing dirty file involved in directory rename' ' + ( + cd 11b && + + git checkout A^0 && + echo stuff >>
[PATCH v10 22/36] merge-recursive: when comparing files, don't include trees
get_renames() would look up stage data that already existed (populated in get_unmerged(), taken from whatever unpack_trees() created), and if it didn't exist, would call insert_stage_data() to create the necessary entry for the given file. The insert_stage_data() fallback becomes much more important for directory rename detection, because that creates a mechanism to have a file in the resulting merge that didn't exist on either side of history. However, insert_stage_data(), due to calling get_tree_entry() loaded up trees as readily as files. We aren't interested in comparing trees to files; the D/F conflict handling is done elsewhere. This code is just concerned with what entries existed for a given path on the different sides of the merge, so create a get_tree_entry_if_blob() helper function and use it. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 27 +-- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index ecead3df4b..d569e3e893 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -421,6 +421,21 @@ static void get_files_dirs(struct merge_options *o, struct tree *tree) read_tree_recursive(tree, "", 0, 0, &match_all, save_files_dirs, o); } +static int get_tree_entry_if_blob(const struct object_id *tree, + const char *path, + struct object_id *hashy, + unsigned int *mode_o) +{ + int ret; + + ret = get_tree_entry(tree, path, hashy, mode_o); + if (S_ISDIR(*mode_o)) { + oidcpy(hashy, &null_oid); + *mode_o = 0; + } + return ret; +} + /* * Returns an index_entry instance which doesn't have to correspond to * a real cache entry in Git's index. @@ -431,12 +446,12 @@ static struct stage_data *insert_stage_data(const char *path, { struct string_list_item *item; struct stage_data *e = xcalloc(1, sizeof(struct stage_data)); - get_tree_entry(&o->object.oid, path, - &e->stages[1].oid, &e->stages[1].mode); - get_tree_entry(&a->object.oid, path, - &e->stages[2].oid, &e->stages[2].mode); - get_tree_entry(&b->object.oid, path, - &e->stages[3].oid, &e->stages[3].mode); + get_tree_entry_if_blob(&o->object.oid, path, + &e->stages[1].oid, &e->stages[1].mode); + get_tree_entry_if_blob(&a->object.oid, path, + &e->stages[2].oid, &e->stages[2].mode); + get_tree_entry_if_blob(&b->object.oid, path, + &e->stages[3].oid, &e->stages[3].mode); item = string_list_insert(entries, path); item->util = e; return e; -- 2.17.0.290.ge988e9ce2a
[PATCH v10 01/36] directory rename detection: basic testcases
Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6043-merge-rename-directories.sh | 442 1 file changed, 442 insertions(+) create mode 100755 t/t6043-merge-rename-directories.sh diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh new file mode 100755 index 00..d045f0e31e --- /dev/null +++ b/t/t6043-merge-rename-directories.sh @@ -0,0 +1,442 @@ +#!/bin/sh + +test_description="recursive merge with directory renames" +# includes checking of many corner cases, with a similar methodology to: +# t6042: corner cases with renames but not criss-cross merges +# t6036: corner cases with both renames and criss-cross merges +# +# The setup for all of them, pictorially, is: +# +# A +# o +# / \ +# O o ? +# \ / +# o +# B +# +# To help make it easier to follow the flow of tests, they have been +# divided into sections and each test will start with a quick explanation +# of what commits O, A, and B contain. +# +# Notation: +#z/{b,c} means files z/b and z/c both exist +#x/d_1 means file x/d exists with content d1. (Purpose of the +# underscore notation is to differentiate different +# files that might be renamed into each other's paths.) + +. ./test-lib.sh + + +### +# SECTION 1: Basic cases we should be able to handle +### + +# Testcase 1a, Basic directory rename. +# Commit O: z/{b,c} +# Commit A: y/{b,c} +# Commit B: z/{b,c,d,e/f} +# Expected: y/{b,c,d,e/f} + +test_expect_success '1a-setup: Simple directory rename detection' ' + test_create_repo 1a && + ( + cd 1a && + + mkdir z && + echo b >z/b && + echo c >z/c && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + git mv z y && + test_tick && + git commit -m "A" && + + git checkout B && + echo d >z/d && + mkdir z/e && + echo f >z/e/f && + git add z/d z/e/f && + test_tick && + git commit -m "B" + ) +' + +test_expect_failure '1a-check: Simple directory rename detection' ' + ( + cd 1a && + + git checkout A^0 && + + git merge -s recursive B^0 && + + git ls-files -s >out && + test_line_count = 4 out && + + git rev-parse >actual \ + HEAD:y/b HEAD:y/c HEAD:y/d HEAD:y/e/f && + git rev-parse >expect \ + O:z/bO:z/cB:z/dB:z/e/f && + test_cmp expect actual && + + git hash-object y/d >actual && + git rev-parse B:z/d >expect && + test_cmp expect actual && + + test_must_fail git rev-parse HEAD:z/d && + test_must_fail git rev-parse HEAD:z/e/f && + test_path_is_missing z/d && + test_path_is_missing z/e/f + ) +' + +# Testcase 1b, Merge a directory with another +# Commit O: z/{b,c}, y/d +# Commit A: z/{b,c,e}, y/d +# Commit B: y/{b,c,d} +# Expected: y/{b,c,d,e} + +test_expect_success '1b-setup: Merge a directory with another' ' + test_create_repo 1b && + ( + cd 1b && + + mkdir z && + echo b >z/b && + echo c >z/c && + mkdir y && + echo d >y/d && + git add z y && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + echo e >z/e && + git add z/e && + test_tick && + git commit -m "A" && + + git checkout B && + git mv z/b y && + git mv z/c y && + rmdir z && + test_tick && + git commit -m "B" + ) +' + +test_expect_failure '1b-check: Merge a directory with another' ' + ( + cd 1b && + + git checkout A^0 && + + git merge -s recursive B^0 && + + git ls-files -s >out && + test_line_count = 4 out && + + git rev-parse >actual \ + HEAD:y/b HEAD:y/c HEAD:y/d HEAD:y/e && + git rev-parse >expect \ + O:z/bO:z/cO:y/dA:z/e && + test_cmp expect actual && + test_must_fail git rev-parse HEAD:z/e + )
[PATCH v10 18/36] merge-recursive: add get_directory_renames()
This populates a set of directory renames for us. The set of directory renames is not yet used, but will be in subsequent commits. Note that the use of a string_list for possible_new_dirs in the new dir_rename_entry struct implies an O(n^2) algorithm; however, in practice I expect the number of distinct directories that files were renamed into from a single original directory to be O(1). My guess is that n has a mode of 1 and a mean of less than 2, so, for now, string_list seems good enough for possible_new_dirs. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 224 +- merge-recursive.h | 18 2 files changed, 239 insertions(+), 3 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 30894c1cc7..22c5e8e5c9 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -49,6 +49,44 @@ static unsigned int path_hash(const char *path) return ignore_case ? strihash(path) : strhash(path); } +static struct dir_rename_entry *dir_rename_find_entry(struct hashmap *hashmap, + char *dir) +{ + struct dir_rename_entry key; + + if (dir == NULL) + return NULL; + hashmap_entry_init(&key, strhash(dir)); + key.dir = dir; + return hashmap_get(hashmap, &key, NULL); +} + +static int dir_rename_cmp(const void *unused_cmp_data, + const void *entry, + const void *entry_or_key, + const void *unused_keydata) +{ + const struct dir_rename_entry *e1 = entry; + const struct dir_rename_entry *e2 = entry_or_key; + + return strcmp(e1->dir, e2->dir); +} + +static void dir_rename_init(struct hashmap *map) +{ + hashmap_init(map, dir_rename_cmp, NULL, 0); +} + +static void dir_rename_entry_init(struct dir_rename_entry *entry, + char *directory) +{ + hashmap_entry_init(entry, strhash(directory)); + entry->dir = directory; + entry->non_unique_new_dir = 0; + strbuf_init(&entry->new_dir, 0); + string_list_init(&entry->possible_new_dirs, 0); +} + static void flush_output(struct merge_options *o) { if (o->buffer_output < 2 && o->obuf.len) { @@ -1357,6 +1395,169 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *o, return ret; } +static void get_renamed_dir_portion(const char *old_path, const char *new_path, + char **old_dir, char **new_dir) +{ + char *end_of_old, *end_of_new; + int old_len, new_len; + + *old_dir = NULL; + *new_dir = NULL; + + /* +* For +*"a/b/c/d/e/foo.c" -> "a/b/some/thing/else/e/foo.c" +* the "e/foo.c" part is the same, we just want to know that +*"a/b/c/d" was renamed to "a/b/some/thing/else" +* so, for this example, this function returns "a/b/c/d" in +* *old_dir and "a/b/some/thing/else" in *new_dir. +* +* Also, if the basename of the file changed, we don't care. We +* want to know which portion of the directory, if any, changed. +*/ + end_of_old = strrchr(old_path, '/'); + end_of_new = strrchr(new_path, '/'); + + if (end_of_old == NULL || end_of_new == NULL) + return; + while (*--end_of_new == *--end_of_old && + end_of_old != old_path && + end_of_new != new_path) + ; /* Do nothing; all in the while loop */ + /* +* We've found the first non-matching character in the directory +* paths. That means the current directory we were comparing +* represents the rename. Move end_of_old and end_of_new back +* to the full directory name. +*/ + if (*end_of_old == '/') + end_of_old++; + if (*end_of_old != '/') + end_of_new++; + end_of_old = strchr(end_of_old, '/'); + end_of_new = strchr(end_of_new, '/'); + + /* +* It may have been the case that old_path and new_path were the same +* directory all along. Don't claim a rename if they're the same. +*/ + old_len = end_of_old - old_path; + new_len = end_of_new - new_path; + + if (old_len != new_len || strncmp(old_path, new_path, old_len)) { + *old_dir = xstrndup(old_path, old_len); + *new_dir = xstrndup(new_path, new_len); + } +} + +static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs, +struct tree *tree) +{ + struct hashmap *dir_renames; + struct hashmap_iter iter; + struct dir_rename_entry *entry; + int i; + + /* +* Typically, we think of a directory rename as all files from a +* certain directory being moved to a target directory. However, +
[PATCH v10 25/36] merge-recursive: fix overwriting dirty files involved in renames
This fixes an issue that existed before my directory rename detection patches that affects both normal renames and renames implied by directory rename detection. Additional codepaths that only affect overwriting of dirty files that are involved in directory rename detection will be added in a subsequent commit. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 85 ++--- merge-recursive.h | 2 + t/t3501-revert-cherry-pick.sh | 2 +- t/t6043-merge-rename-directories.sh | 2 +- t/t7607-merge-overwrite.sh | 2 +- unpack-trees.c | 4 +- unpack-trees.h | 4 ++ 7 files changed, 77 insertions(+), 24 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index c1c4faf61e..7fdcba4f22 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -337,32 +337,37 @@ static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree) init_tree_desc(desc, tree->buffer, tree->size); } -static int git_merge_trees(int index_only, +static int git_merge_trees(struct merge_options *o, struct tree *common, struct tree *head, struct tree *merge) { int rc; struct tree_desc t[3]; - struct unpack_trees_options opts; - memset(&opts, 0, sizeof(opts)); - if (index_only) - opts.index_only = 1; + memset(&o->unpack_opts, 0, sizeof(o->unpack_opts)); + if (o->call_depth) + o->unpack_opts.index_only = 1; else - opts.update = 1; - opts.merge = 1; - opts.head_idx = 2; - opts.fn = threeway_merge; - opts.src_index = &the_index; - opts.dst_index = &the_index; - setup_unpack_trees_porcelain(&opts, "merge"); + o->unpack_opts.update = 1; + o->unpack_opts.merge = 1; + o->unpack_opts.head_idx = 2; + o->unpack_opts.fn = threeway_merge; + o->unpack_opts.src_index = &the_index; + o->unpack_opts.dst_index = &the_index; + setup_unpack_trees_porcelain(&o->unpack_opts, "merge"); init_tree_desc_from_tree(t+0, common); init_tree_desc_from_tree(t+1, head); init_tree_desc_from_tree(t+2, merge); - rc = unpack_trees(3, t, &opts); + rc = unpack_trees(3, t, &o->unpack_opts); + /* +* unpack_trees NULLifies src_index, but it's used in verify_uptodate, +* so set to the new index which will usually have modification +* timestamp info copied over. +*/ + o->unpack_opts.src_index = &the_index; cache_tree_free(&active_cache_tree); return rc; } @@ -795,6 +800,20 @@ static int would_lose_untracked(const char *path) return !was_tracked(path) && file_exists(path); } +static int was_dirty(struct merge_options *o, const char *path) +{ + struct cache_entry *ce; + int dirty = 1; + + if (o->call_depth || !was_tracked(path)) + return !dirty; + + ce = cache_file_exists(path, strlen(path), ignore_case); + dirty = (ce->ce_stat_data.sd_mtime.sec > 0 && +verify_uptodate(ce, &o->unpack_opts) != 0); + return dirty; +} + static int make_room_for_path(struct merge_options *o, const char *path) { int status, i; @@ -2687,6 +2706,7 @@ static int handle_modify_delete(struct merge_options *o, static int merge_content(struct merge_options *o, const char *path, +int file_in_way, struct object_id *o_oid, int o_mode, struct object_id *a_oid, int a_mode, struct object_id *b_oid, int b_mode, @@ -2761,7 +2781,7 @@ static int merge_content(struct merge_options *o, return -1; } - if (df_conflict_remains) { + if (df_conflict_remains || file_in_way) { char *new_path; if (o->call_depth) { remove_file_from_cache(path); @@ -2795,6 +2815,30 @@ static int merge_content(struct merge_options *o, return mfi.clean; } +static int conflict_rename_normal(struct merge_options *o, + const char *path, + struct object_id *o_oid, unsigned int o_mode, + struct object_id *a_oid, unsigned int a_mode, + struct object_id *b_oid, unsigned int b_mode, + struct rename_conflict_info *ci) +{ + int clean_merge; + int file_in_the_way = 0; + + if (was_dirty(o, path)) { + file_in_the_way = 1; + output(o, 1, _("Refusing to lose dirty file at %s"), path); + } + + /* Merge the content and write it out */ + clean_merge
[PATCH v10 02/36] directory rename detection: directory splitting testcases
Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6043-merge-rename-directories.sh | 143 1 file changed, 143 insertions(+) diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index d045f0e31e..b22a9052b3 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -439,4 +439,147 @@ test_expect_failure '1f-check: Split a directory into two other directories' ' # in section 2, plus testcases 3a and 4a. ### + +### +# SECTION 2: Split into multiple directories, with equal number of paths +# +# Explore the splitting-a-directory rules a bit; what happens in the +# edge cases? +# +# Note that there is a closely related case of a directory not being +# split on either side of history, but being renamed differently on +# each side. See testcase 8e for that. +### + +# Testcase 2a, Directory split into two on one side, with equal numbers of paths +# Commit O: z/{b,c} +# Commit A: y/b, w/c +# Commit B: z/{b,c,d} +# Expected: y/b, w/c, z/d, with warning about z/ -> (y/ vs. w/) conflict +test_expect_success '2a-setup: Directory split into two on one side, with equal numbers of paths' ' + test_create_repo 2a && + ( + cd 2a && + + mkdir z && + echo b >z/b && + echo c >z/c && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + mkdir y && + mkdir w && + git mv z/b y/ && + git mv z/c w/ && + test_tick && + git commit -m "A" && + + git checkout B && + echo d >z/d && + git add z/d && + test_tick && + git commit -m "B" + ) +' + +test_expect_failure '2a-check: Directory split into two on one side, with equal numbers of paths' ' + ( + cd 2a && + + git checkout A^0 && + + test_must_fail git merge -s recursive B^0 >out && + test_i18ngrep "CONFLICT.*directory rename split" out && + + git ls-files -s >out && + test_line_count = 3 out && + git ls-files -u >out && + test_line_count = 0 out && + git ls-files -o >out && + test_line_count = 1 out && + + git rev-parse >actual \ + :0:y/b :0:w/c :0:z/d && + git rev-parse >expect \ +O:z/b O:z/c B:z/d && + test_cmp expect actual + ) +' + +# Testcase 2b, Directory split into two on one side, with equal numbers of paths +# Commit O: z/{b,c} +# Commit A: y/b, w/c +# Commit B: z/{b,c}, x/d +# Expected: y/b, w/c, x/d; No warning about z/ -> (y/ vs. w/) conflict +test_expect_success '2b-setup: Directory split into two on one side, with equal numbers of paths' ' + test_create_repo 2b && + ( + cd 2b && + + mkdir z && + echo b >z/b && + echo c >z/c && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + mkdir y && + mkdir w && + git mv z/b y/ && + git mv z/c w/ && + test_tick && + git commit -m "A" && + + git checkout B && + mkdir x && + echo d >x/d && + git add x/d && + test_tick && + git commit -m "B" + ) +' + +test_expect_success '2b-check: Directory split into two on one side, with equal numbers of paths' ' + ( + cd 2b && + + git checkout A^0 && + + git merge -s recursive B^0 >out && + + git ls-files -s >out && + test_line_count = 3 out && + git ls-files -u >out && + test_line_count = 0 out && + git ls-files -o >out && + test_line_count = 1 out && + + git rev-parse >actual \ + :0:y/b :0:w/c :0:x/d && + git rev-parse >expect \ +O:z/b O:z/c B:x/d && + test_cmp expect actual && + test_i18ngrep ! "CONFLICT.*directory rename split" out + ) +' + +### +# Rules su
[PATCH v10 21/36] merge-recursive: check for file level conflicts then get new name
Before trying to apply directory renames to paths within the given directories, we want to make sure that there aren't conflicts at the file level either. If there aren't any, then get the new name from any directory renames. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 174 ++-- strbuf.c| 16 +++ strbuf.h| 16 +++ t/t6043-merge-rename-directories.sh | 2 +- 4 files changed, 199 insertions(+), 9 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 25ea6841fc..ecead3df4b 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1520,6 +1520,91 @@ static void remove_hashmap_entries(struct hashmap *dir_renames, string_list_clear(items_to_remove, 0); } +/* + * See if there is a directory rename for path, and if there are any file + * level conflicts for the renamed location. If there is a rename and + * there are no conflicts, return the new name. Otherwise, return NULL. + */ +static char *handle_path_level_conflicts(struct merge_options *o, +const char *path, +struct dir_rename_entry *entry, +struct hashmap *collisions, +struct tree *tree) +{ + char *new_path = NULL; + struct collision_entry *collision_ent; + int clean = 1; + struct strbuf collision_paths = STRBUF_INIT; + + /* +* entry has the mapping of old directory name to new directory name +* that we want to apply to path. +*/ + new_path = apply_dir_rename(entry, path); + + if (!new_path) { + /* This should only happen when entry->non_unique_new_dir set */ + if (!entry->non_unique_new_dir) + BUG("entry->non_unqiue_dir not set and !new_path"); + output(o, 1, _("CONFLICT (directory rename split): " + "Unclear where to place %s because directory " + "%s was renamed to multiple other directories, " + "with no destination getting a majority of the " + "files."), + path, entry->dir); + clean = 0; + return NULL; + } + + /* +* The caller needs to have ensured that it has pre-populated +* collisions with all paths that map to new_path. Do a quick check +* to ensure that's the case. +*/ + collision_ent = collision_find_entry(collisions, new_path); + if (collision_ent == NULL) + BUG("collision_ent is NULL"); + + /* +* Check for one-sided add/add/.../add conflicts, i.e. +* where implicit renames from the other side doing +* directory rename(s) can affect this side of history +* to put multiple paths into the same location. Warn +* and bail on directory renames for such paths. +*/ + if (collision_ent->reported_already) { + clean = 0; + } else if (tree_has_path(tree, new_path)) { + collision_ent->reported_already = 1; + strbuf_add_separated_string_list(&collision_paths, ", ", +&collision_ent->source_files); + output(o, 1, _("CONFLICT (implicit dir rename): Existing " + "file/dir at %s in the way of implicit " + "directory rename(s) putting the following " + "path(s) there: %s."), + new_path, collision_paths.buf); + clean = 0; + } else if (collision_ent->source_files.nr > 1) { + collision_ent->reported_already = 1; + strbuf_add_separated_string_list(&collision_paths, ", ", +&collision_ent->source_files); + output(o, 1, _("CONFLICT (implicit dir rename): Cannot map " + "more than one path to %s; implicit directory " + "renames tried to put these paths there: %s"), + new_path, collision_paths.buf); + clean = 0; + } + + /* Free memory we no longer need */ + strbuf_release(&collision_paths); + if (!clean && new_path) { + free(new_path); + return NULL; + } + + return new_path; +} + /* * There are a couple things we want to do at the directory level: * 1. Check for both sides renaming to the same thing, in order to avoid @@ -1799,6 +1884,59 @@ static void compute_collisions(struct hashmap *collisions, } } +static char *check_for_directory_rename(struct merge_options *o, +
[PATCH v10 09/36] directory rename detection: miscellaneous testcases to complete coverage
I came up with the testcases in the first eight sections before coding up the implementation. The testcases in this section were mostly ones I thought of while coding/debugging, and which I was too lazy to insert into the previous sections because I didn't want to re-label with all the testcase references. :-) Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6043-merge-rename-directories.sh | 565 +++- 1 file changed, 564 insertions(+), 1 deletion(-) diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index e211e8ca31..cbbb949014 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -305,6 +305,7 @@ test_expect_failure '1d-check: Directory renames cause a rename/rename(2to1) con ' # Testcase 1e, Renamed directory, with all filenames being renamed too +# (Related to testcases 9f & 9g) # Commit O: z/{oldb,oldc} # Commit A: y/{newb,newc} # Commit B: z/{oldb,oldc,d} @@ -593,7 +594,7 @@ test_expect_success '2b-check: Directory split into two on one side, with equal ### # Testcase 3a, Avoid implicit rename if involved as source on other side -# (Related to testcases 1c and 1f) +# (Related to testcases 1c, 1f, and 9h) # Commit O: z/{b,c,d} # Commit A: z/{b,c,d} (no change) # Commit B: y/{b,c}, x/d @@ -2316,4 +2317,566 @@ test_expect_failure '8e-check: Both sides rename, one side adds to original dire ) ' +### +# SECTION 9: Other testcases +# +# This section consists of miscellaneous testcases I thought of during +# the implementation which round out the testing. +### + +# Testcase 9a, Inner renamed directory within outer renamed directory +# (Related to testcase 1f) +# Commit O: z/{b,c,d/{e,f,g}} +# Commit A: y/{b,c}, x/w/{e,f,g} +# Commit B: z/{b,c,d/{e,f,g,h},i} +# Expected: y/{b,c,i}, x/w/{e,f,g,h} +# NOTE: The only reason this one is interesting is because when a directory +# is split into multiple other directories, we determine by the weight +# of which one had the most paths going to it. A naive implementation +# of that could take the new file in commit B at z/i to x/w/i or x/i. + +test_expect_success '9a-setup: Inner renamed directory within outer renamed directory' ' + test_create_repo 9a && + ( + cd 9a && + + mkdir -p z/d && + echo b >z/b && + echo c >z/c && + echo e >z/d/e && + echo f >z/d/f && + echo g >z/d/g && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + mkdir x && + git mv z/d x/w && + git mv z y && + test_tick && + git commit -m "A" && + + git checkout B && + echo h >z/d/h && + echo i >z/i && + git add z && + test_tick && + git commit -m "B" + ) +' + +test_expect_failure '9a-check: Inner renamed directory within outer renamed directory' ' + ( + cd 9a && + + git checkout A^0 && + + git merge -s recursive B^0 && + + git ls-files -s >out && + test_line_count = 7 out && + git ls-files -u >out && + test_line_count = 0 out && + git ls-files -o >out && + test_line_count = 1 out && + + git rev-parse >actual \ + HEAD:y/b HEAD:y/c HEAD:y/i && + git rev-parse >expect \ + O:z/bO:z/cB:z/i && + test_cmp expect actual && + + git rev-parse >actual \ + HEAD:x/w/e HEAD:x/w/f HEAD:x/w/g HEAD:x/w/h && + git rev-parse >expect \ + O:z/d/eO:z/d/fO:z/d/gB:z/d/h && + test_cmp expect actual + ) +' + +# Testcase 9b, Transitive rename with content merge +# (Related to testcase 1c) +# Commit O: z/{b,c}, x/d_1 +# Commit A: y/{b,c}, x/d_2 +# Commit B: z/{b,c,d_3} +# Expected: y/{b,c,d_merged} + +test_expect_success '9b-setup: Transitive rename with content merge' ' + test_create_repo 9b && + ( + cd 9b && + + mkdir z && + echo b >z/b && + echo c >z/c && + mkdir x && + test_seq 1 10 >x/d && + git add z x && + test_tick && + git commit -m "O" && + + gi
[PATCH v10 13/36] merge-recursive: introduce new functions to handle rename logic
The amount of logic in merge_trees() relative to renames was just a few lines, but split it out into new handle_renames() and cleanup_renames() functions to prepare for additional logic to be added to each. No code or logic changes, just a new place to put stuff for when the rename detection gains additional checks. Note that process_renames() records pointers to various information (such as diff_filepairs) into rename_conflict_info structs. Even though the rename string_lists are not directly used once handle_renames() completes, we should not immediately free the lists at the end of that function because they store the information referenced in the rename_conflict_info, which is used later in process_entry(). Thus the reason for a separate cleanup_renames(). Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 43 +-- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 973b6e2985..40e142efdb 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1646,6 +1646,32 @@ static int process_renames(struct merge_options *o, return clean_merge; } +struct rename_info { + struct string_list *head_renames; + struct string_list *merge_renames; +}; + +static int handle_renames(struct merge_options *o, + struct tree *common, + struct tree *head, + struct tree *merge, + struct string_list *entries, + struct rename_info *ri) +{ + ri->head_renames = get_renames(o, head, common, head, merge, entries); + ri->merge_renames = get_renames(o, merge, common, head, merge, entries); + return process_renames(o, ri->head_renames, ri->merge_renames); +} + +static void cleanup_renames(struct rename_info *re_info) +{ + string_list_clear(re_info->head_renames, 0); + string_list_clear(re_info->merge_renames, 0); + + free(re_info->head_renames); + free(re_info->merge_renames); +} + static struct object_id *stage_oid(const struct object_id *oid, unsigned mode) { return (is_null_oid(oid) || mode == 0) ? NULL: (struct object_id *)oid; @@ -2005,7 +2031,8 @@ int merge_trees(struct merge_options *o, } if (unmerged_cache()) { - struct string_list *entries, *re_head, *re_merge; + struct string_list *entries; + struct rename_info re_info; int i; /* * Only need the hashmap while processing entries, so @@ -2019,9 +2046,8 @@ int merge_trees(struct merge_options *o, get_files_dirs(o, merge); entries = get_unmerged(); - re_head = get_renames(o, head, common, head, merge, entries); - re_merge = get_renames(o, merge, common, head, merge, entries); - clean = process_renames(o, re_head, re_merge); + clean = handle_renames(o, common, head, merge, entries, + &re_info); record_df_conflict_files(o, entries); if (clean < 0) goto cleanup; @@ -2046,16 +2072,13 @@ int merge_trees(struct merge_options *o, } cleanup: - string_list_clear(re_merge, 0); - string_list_clear(re_head, 0); + cleanup_renames(&re_info); + string_list_clear(entries, 1); + free(entries); hashmap_free(&o->current_file_dir_set, 1); - free(re_merge); - free(re_head); - free(entries); - if (clean < 0) return clean; } -- 2.17.0.290.ge988e9ce2a
[PATCH v10 04/36] directory rename detection: partially renamed directory testcase/discussion
Add a long note about why we are not considering "partial directory renames" for the current directory rename detection implementation. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6043-merge-rename-directories.sh | 115 1 file changed, 115 insertions(+) diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index 8049ed5fc9..713ad2b75e 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -735,4 +735,119 @@ test_expect_success '3b-check: Avoid implicit rename if involved as source on cu # of a rename on either side of a merge. ### + +### +# SECTION 4: Partially renamed directory; still exists on both sides of merge +# +# What if we were to attempt to do directory rename detection when someone +# "mostly" moved a directory but still left some files around, or, +# equivalently, fully renamed a directory in one commmit and then recreated +# that directory in a later commit adding some new files and then tried to +# merge? +# +# It's hard to divine user intent in these cases, because you can make an +# argument that, depending on the intermediate history of the side being +# merged, that some users will want files in that directory to +# automatically be detected and renamed, while users with a different +# intermediate history wouldn't want that rename to happen. +# +# I think that it is best to simply not have directory rename detection +# apply to such cases. My reasoning for this is four-fold: (1) it's +# easiest for users in general to figure out what happened if we don't +# apply directory rename detection in any such case, (2) it's an easy rule +# to explain ["We don't do directory rename detection if the directory +# still exists on both sides of the merge"], (3) we can get some hairy +# edge/corner cases that would be really confusing and possibly not even +# representable in the index if we were to even try, and [related to 3] (4) +# attempting to resolve this issue of divining user intent by examining +# intermediate history goes against the spirit of three-way merges and is a +# path towards crazy corner cases that are far more complex than what we're +# already dealing with. +# +# Note that the wording of the rule ("We don't do directory rename +# detection if the directory still exists on both sides of the merge.") +# also excludes "renaming" of a directory into a subdirectory of itself +# (e.g. /some/dir/* -> /some/dir/subdir/*). It may be possible to carve +# out an exception for "renaming"-beneath-itself cases without opening +# weird edge/corner cases for other partial directory renames, but for now +# we are keeping the rule simple. +# +# This section contains a test for a partially-renamed-directory case. +### + +# Testcase 4a, Directory split, with original directory still present +# (Related to testcase 1f) +# Commit O: z/{b,c,d,e} +# Commit A: y/{b,c,d}, z/e +# Commit B: z/{b,c,d,e,f} +# Expected: y/{b,c,d}, z/{e,f} +# NOTE: Even though most files from z moved to y, we don't want f to follow. + +test_expect_success '4a-setup: Directory split, with original directory still present' ' + test_create_repo 4a && + ( + cd 4a && + + mkdir z && + echo b >z/b && + echo c >z/c && + echo d >z/d && + echo e >z/e && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + mkdir y && + git mv z/b y/ && + git mv z/c y/ && + git mv z/d y/ && + test_tick && + git commit -m "A" && + + git checkout B && + echo f >z/f && + git add z/f && + test_tick && + git commit -m "B" + ) +' + +test_expect_success '4a-check: Directory split, with original directory still present' ' + ( + cd 4a && + + git checkout A^0 && + + git merge -s recursive B^0 && + + git ls-files -s >out && + test_line_count = 5 out && + git ls-files -u >out && + test_line_count = 0 out && + git ls-files -o >out && + test_line_count = 1 out && + + git rev-parse >actual \ + HEAD:y/b HEAD:y/c HEAD:y/d HEAD:z/e HEAD:z/f && + git rev-parse >expect \ + O:z/bO:z/cO:z/dO:z/eB:z/f && + test_cmp expect actual +
[PATCH v10 08/36] directory rename detection: testcases exploring possibly suboptimal merges
Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6043-merge-rename-directories.sh | 404 1 file changed, 404 insertions(+) diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index 6db1439675..e211e8ca31 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -1912,4 +1912,408 @@ test_expect_failure '7e-check: transitive rename in rename/delete AND dirs in th ) ' +### +# SECTION 8: Suboptimal merges +# +# As alluded to in the last section, the ruleset we have built up for +# detecting directory renames unfortunately has some special cases where it +# results in slightly suboptimal or non-intuitive behavior. This section +# explores these cases. +# +# To be fair, we already had non-intuitive or suboptimal behavior for most +# of these cases in git before introducing implicit directory rename +# detection, but it'd be nice if there was a modified ruleset out there +# that handled these cases a bit better. +### + +# Testcase 8a, Dual-directory rename, one into the others' way +# Commit O. x/{a,b}, y/{c,d} +# Commit A. x/{a,b,e}, y/{c,d,f} +# Commit B. y/{a,b}, z/{c,d} +# +# Possible Resolutions: +# w/o dir-rename detection: y/{a,b,f}, z/{c,d}, x/e +# Currently expected: y/{a,b,e,f}, z/{c,d} +# Optimal: y/{a,b,e}, z/{c,d,f} +# +# Note: Both x and y got renamed and it'd be nice to detect both, and we do +# better with directory rename detection than git did without, but the +# simple rule from section 5 prevents me from handling this as optimally as +# we potentially could. + +test_expect_success '8a-setup: Dual-directory rename, one into the others way' ' + test_create_repo 8a && + ( + cd 8a && + + mkdir x && + mkdir y && + echo a >x/a && + echo b >x/b && + echo c >y/c && + echo d >y/d && + git add x y && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + echo e >x/e && + echo f >y/f && + git add x/e y/f && + test_tick && + git commit -m "A" && + + git checkout B && + git mv y z && + git mv x y && + test_tick && + git commit -m "B" + ) +' + +test_expect_failure '8a-check: Dual-directory rename, one into the others way' ' + ( + cd 8a && + + git checkout A^0 && + + git merge -s recursive B^0 && + + git ls-files -s >out && + test_line_count = 6 out && + git ls-files -u >out && + test_line_count = 0 out && + git ls-files -o >out && + test_line_count = 1 out && + + git rev-parse >actual \ + HEAD:y/a HEAD:y/b HEAD:y/e HEAD:y/f HEAD:z/c HEAD:z/d && + git rev-parse >expect \ + O:x/aO:x/bA:x/eA:y/fO:y/cO:y/d && + test_cmp expect actual + ) +' + +# Testcase 8b, Dual-directory rename, one into the others' way, with conflicting filenames +# Commit O. x/{a_1,b_1}, y/{a_2,b_2} +# Commit A. x/{a_1,b_1,e_1}, y/{a_2,b_2,e_2} +# Commit B. y/{a_1,b_1}, z/{a_2,b_2} +# +# w/o dir-rename detection: y/{a_1,b_1,e_2}, z/{a_2,b_2}, x/e_1 +# Currently expected: +# Scary:y/{a_1,b_1}, z/{a_2,b_2}, CONFLICT(add/add, e_1 vs. e_2) +# Optimal: y/{a_1,b_1,e_1}, z/{a_2,b_2,e_2} +# +# Note: Very similar to 8a, except instead of 'e' and 'f' in directories x and +# y, both are named 'e'. Without directory rename detection, neither file +# moves directories. Implement directory rename detection suboptimally, and +# you get an add/add conflict, but both files were added in commit A, so this +# is an add/add conflict where one side of history added both files -- +# something we can't represent in the index. Obviously, we'd prefer the last +# resolution, but our previous rules are too coarse to allow it. Using both +# the rules from section 4 and section 5 save us from the Scary resolution, +# making us fall back to pre-directory-rename-detection behavior for both +# e_1 and e_2. + +test_expect_success '8b-setup: Dual-directory rename, one into the others way, with conflicting filenames' ' + test_create_repo 8b && + ( + cd 8b && + + mkdir x && + mkdir y && + echo a1 >x/a && + echo b1 >x/b && +
[PATCH v10 17/36] merge-recursive: make a helper function for cleanup for handle_renames
In anticipation of more involved cleanup to come, make a helper function for doing the cleanup at the end of handle_renames. Rename the already existing cleanup_rename[s]() to final_cleanup_rename[s](), name the new helper initial_cleanup_rename(), and leave the big comment in the code about why we can't do all the cleanup at once. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 23 +-- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 55a8ace948..30894c1cc7 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1668,6 +1668,12 @@ struct rename_info { struct string_list *merge_renames; }; +static void initial_cleanup_rename(struct diff_queue_struct *pairs) +{ + free(pairs->queue); + free(pairs); +} + static int handle_renames(struct merge_options *o, struct tree *common, struct tree *head, @@ -1698,16 +1704,13 @@ static int handle_renames(struct merge_options *o, * data structures are still needed and referenced in * process_entry(). But there are a few things we can free now. */ - - free(head_pairs->queue); - free(head_pairs); - free(merge_pairs->queue); - free(merge_pairs); + initial_cleanup_rename(head_pairs); + initial_cleanup_rename(merge_pairs); return clean; } -static void cleanup_rename(struct string_list *rename) +static void final_cleanup_rename(struct string_list *rename) { const struct rename *re; int i; @@ -1723,10 +1726,10 @@ static void cleanup_rename(struct string_list *rename) free(rename); } -static void cleanup_renames(struct rename_info *re_info) +static void final_cleanup_renames(struct rename_info *re_info) { - cleanup_rename(re_info->head_renames); - cleanup_rename(re_info->merge_renames); + final_cleanup_rename(re_info->head_renames); + final_cleanup_rename(re_info->merge_renames); } static struct object_id *stage_oid(const struct object_id *oid, unsigned mode) @@ -2129,7 +2132,7 @@ int merge_trees(struct merge_options *o, } cleanup: - cleanup_renames(&re_info); + final_cleanup_renames(&re_info); string_list_clear(entries, 1); free(entries); -- 2.17.0.290.ge988e9ce2a
[PATCH v10 06/36] directory rename detection: testcases checking which side did the rename
Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6043-merge-rename-directories.sh | 336 1 file changed, 336 insertions(+) diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index b469c807c2..9ae245a522 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -1180,4 +1180,340 @@ test_expect_failure '5d-check: Directory/file/file conflict due to directory ren # back to old handling. But, sadly, see testcases 8a and 8b. ### + +### +# SECTION 6: Same side of the merge was the one that did the rename +# +# It may sound obvious that you only want to apply implicit directory +# renames to directories if the _other_ side of history did the renaming. +# If you did make an implementation that didn't explicitly enforce this +# rule, the majority of cases that would fall under this section would +# also be solved by following the rules from the above sections. But +# there are still a few that stick out, so this section covers them just +# to make sure we also get them right. +### + +# Testcase 6a, Tricky rename/delete +# Commit O: z/{b,c,d} +# Commit A: z/b +# Commit B: y/{b,c}, z/d +# Expected: y/b, CONFLICT(rename/delete, z/c -> y/c vs. NULL) +# Note: We're just checking here that the rename of z/b and z/c to put +# them under y/ doesn't accidentally catch z/d and make it look like +# it is also involved in a rename/delete conflict. + +test_expect_success '6a-setup: Tricky rename/delete' ' + test_create_repo 6a && + ( + cd 6a && + + mkdir z && + echo b >z/b && + echo c >z/c && + echo d >z/d && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + git rm z/c && + git rm z/d && + test_tick && + git commit -m "A" && + + git checkout B && + mkdir y && + git mv z/b y/ && + git mv z/c y/ && + test_tick && + git commit -m "B" + ) +' + +test_expect_success '6a-check: Tricky rename/delete' ' + ( + cd 6a && + + git checkout A^0 && + + test_must_fail git merge -s recursive B^0 >out && + test_i18ngrep "CONFLICT (rename/delete).*z/c.*y/c" out && + + git ls-files -s >out && + test_line_count = 2 out && + git ls-files -u >out && + test_line_count = 1 out && + git ls-files -o >out && + test_line_count = 1 out && + + git rev-parse >actual \ + :0:y/b :3:y/c && + git rev-parse >expect \ +O:z/b O:z/c && + test_cmp expect actual + ) +' + +# Testcase 6b, Same rename done on both sides +# (Related to testcases 6c and 8e) +# Commit O: z/{b,c} +# Commit A: y/{b,c} +# Commit B: y/{b,c}, z/d +# Expected: y/{b,c}, z/d +# Note: If we did directory rename detection here, we'd move z/d into y/, +# but B did that rename and still decided to put the file into z/, +# so we probably shouldn't apply directory rename detection for it. + +test_expect_success '6b-setup: Same rename done on both sides' ' + test_create_repo 6b && + ( + cd 6b && + + mkdir z && + echo b >z/b && + echo c >z/c && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + git mv z y && + test_tick && + git commit -m "A" && + + git checkout B && + git mv z y && + mkdir z && + echo d >z/d && + git add z/d && + test_tick && + git commit -m "B" + ) +' + +test_expect_success '6b-check: Same rename done on both sides' ' + ( + cd 6b && + + git checkout A^0 && + + git merge -s recursive B^0 && + + git ls-files -s >out && + test_line_count = 3 out && + git ls-files -u >out && + test_line_count = 0 out && + git ls-files -o >out && + test_line_count = 1 out && + + git rev-parse >actual \ +
[PATCH v10 10/36] directory rename detection: tests for handling overwriting untracked files
Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6043-merge-rename-directories.sh | 367 1 file changed, 367 insertions(+) diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index cbbb949014..a6cd38336c 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -2879,4 +2879,371 @@ test_expect_failure '9g-check: Renamed directory that only contained immediate s # side of history for any implicit directory renames. ### +### +# SECTION 10: Handling untracked files +# +# unpack_trees(), upon which the recursive merge algorithm is based, aborts +# the operation if untracked or dirty files would be deleted or overwritten +# by the merge. Unfortunately, unpack_trees() does not understand renames, +# and if it doesn't abort, then it muddies up the working directory before +# we even get to the point of detecting renames, so we need some special +# handling, at least in the case of directory renames. +### + +# Testcase 10a, Overwrite untracked: normal rename/delete +# Commit O: z/{b,c_1} +# Commit A: z/b + untracked z/c + untracked z/d +# Commit B: z/{b,d_1} +# Expected: Aborted Merge + +# ERROR_MSG(untracked working tree files would be overwritten by merge) + +test_expect_success '10a-setup: Overwrite untracked with normal rename/delete' ' + test_create_repo 10a && + ( + cd 10a && + + mkdir z && + echo b >z/b && + echo c >z/c && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + git rm z/c && + test_tick && + git commit -m "A" && + + git checkout B && + git mv z/c z/d && + test_tick && + git commit -m "B" + ) +' + +test_expect_success '10a-check: Overwrite untracked with normal rename/delete' ' + ( + cd 10a && + + git checkout A^0 && + echo very >z/c && + echo important >z/d && + + test_must_fail git merge -s recursive B^0 >out 2>err && + test_i18ngrep "The following untracked working tree files would be overwritten by merge" err && + + git ls-files -s >out && + test_line_count = 1 out && + git ls-files -o >out && + test_line_count = 4 out && + + echo very >expect && + test_cmp expect z/c && + + echo important >expect && + test_cmp expect z/d && + + git rev-parse HEAD:z/b >actual && + git rev-parse O:z/b >expect && + test_cmp expect actual + ) +' + +# Testcase 10b, Overwrite untracked: dir rename + delete +# Commit O: z/{b,c_1} +# Commit A: y/b + untracked y/{c,d,e} +# Commit B: z/{b,d_1,e} +# Expected: Failed Merge; y/b + untracked y/c + untracked y/d on disk + +# z/c_1 -> z/d_1 rename recorded at stage 3 for y/d + +# ERROR_MSG(refusing to lose untracked file at 'y/d') + +test_expect_success '10b-setup: Overwrite untracked with dir rename + delete' ' + test_create_repo 10b && + ( + cd 10b && + + mkdir z && + echo b >z/b && + echo c >z/c && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + git rm z/c && + git mv z/ y/ && + test_tick && + git commit -m "A" && + + git checkout B && + git mv z/c z/d && + echo e >z/e && + git add z/e && + test_tick && + git commit -m "B" + ) +' + +test_expect_failure '10b-check: Overwrite untracked with dir rename + delete' ' + ( + cd 10b && + + git checkout A^0 && + echo very >y/c && + echo important >y/d && + echo contents >y/e && + + test_must_fail git merge -s recursive B^0 >out 2>err && + test_i18ngrep "CONFLICT (rename/delete).*Version B\^0 of y/d left in tree at y/d~B\^0" out && + test_i18ngrep "Error: Refusing to lose untracked file at y/e; writing to y/e~B\^0 instead" out && + + git ls-files -s >out && + test_line_count = 3 out && +
[PATCH v10 27/36] directory rename detection: new testcases showcasing a pair of bugs
Add a testcase showing spurious rename/rename(1to2) conflicts occurring due to directory rename detection. Also add a pair of testcases dealing with moving directory hierarchies around that were suggested by Stefan Beller as "food for thought" during his review of an earlier patch series, but which actually uncovered a bug. Round things out with a test that is a cross between the two testcases that showed existing bugs in order to make sure we aren't merely addressing problems in isolation but in general. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6043-merge-rename-directories.sh | 296 1 file changed, 296 insertions(+) diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index 33e2649824..5b84591445 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -159,6 +159,7 @@ test_expect_success '1b-check: Merge a directory with another' ' # Testcase 1c, Transitive renaming # (Related to testcases 3a and 6d -- when should a transitive rename apply?) # (Related to testcases 9c and 9d -- can transitivity repeat?) +# (Related to testcase 12b -- joint-transitivity?) # Commit O: z/{b,c}, x/d # Commit A: y/{b,c}, x/d # Commit B: z/{b,c,d} @@ -2871,6 +2872,68 @@ test_expect_failure '9g-check: Renamed directory that only contained immediate s ) ' +# Testcase 9h, Avoid implicit rename if involved as source on other side +# (Extremely closely related to testcase 3a) +# Commit O: z/{b,c,d_1} +# Commit A: z/{b,c,d_2} +# Commit B: y/{b,c}, x/d_1 +# Expected: y/{b,c}, x/d_2 +# NOTE: If we applied the z/ -> y/ rename to z/d, then we'd end up with +# a rename/rename(1to2) conflict (z/d -> y/d vs. x/d) +test_expect_success '9h-setup: Avoid dir rename on merely modified path' ' + test_create_repo 9h && + ( + cd 9h && + + mkdir z && + echo b >z/b && + echo c >z/c && + printf "1\n2\n3\n4\n5\n6\n7\n8\nd\n" >z/d && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + test_tick && + echo more >>z/d && + git add z/d && + git commit -m "A" && + + git checkout B && + mkdir y && + mkdir x && + git mv z/b y/ && + git mv z/c y/ && + git mv z/d x/ && + rmdir z && + test_tick && + git commit -m "B" + ) +' + +test_expect_failure '9h-check: Avoid dir rename on merely modified path' ' + ( + cd 9h && + + git checkout A^0 && + + git merge -s recursive B^0 && + + git ls-files -s >out && + test_line_count = 3 out && + + git rev-parse >actual \ + HEAD:y/b HEAD:y/c HEAD:x/d && + git rev-parse >expect \ + O:z/bO:z/cA:z/d && + test_cmp expect actual + ) +' + ### # Rules suggested by section 9: # @@ -3704,4 +3767,237 @@ test_expect_success '11f-check: Avoid deleting not-uptodate with dir rename/rena ) ' +### +# SECTION 12: Everything else +# +# Tests suggested by others. Tests added after implementation completed +# and submitted. Grab bag. +### + +# Testcase 12a, Moving one directory hierarchy into another +# (Related to testcase 9a) +# Commit O: node1/{leaf1,leaf2}, node2/{leaf3,leaf4} +# Commit A: node1/{leaf1,leaf2,node2/{leaf3,leaf4}} +# Commit B: node1/{leaf1,leaf2,leaf5}, node2/{leaf3,leaf4,leaf6} +# Expected: node1/{leaf1,leaf2,leaf5,node2/{leaf3,leaf4,leaf6}} + +test_expect_success '12a-setup: Moving one directory hierarchy into another' ' + test_create_repo 12a && + ( + cd 12a && + + mkdir -p node1 node2 && + echo leaf1 >node1/leaf1 && + echo leaf2 >node1/leaf2 && + echo leaf3 >node2/leaf3 && + echo leaf4 >node2/leaf4 && + git add node1 node2 && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + git mv node2/ node1/ && + test_tick && + git commit -m "A" && + + git checkout B && + echo leaf5 >node1/leaf5 && + echo leaf6 >node2/leaf6 && +
[PATCH v10 15/36] merge-recursive: make !o->detect_rename codepath more obvious
Previously, if !o->detect_rename then get_renames() would return an empty string_list, and then process_renames() would have nothing to iterate over. It seems more straightforward to simply avoid calling either function in that case. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index fc96653f63..5da60b9516 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1339,8 +1339,6 @@ static struct string_list *get_renames(struct merge_options *o, struct diff_options opts; renames = xcalloc(1, sizeof(struct string_list)); - if (!o->detect_rename) - return renames; diff_setup(&opts); opts.flags.recursive = 1; @@ -1658,6 +1656,12 @@ static int handle_renames(struct merge_options *o, struct string_list *entries, struct rename_info *ri) { + ri->head_renames = NULL; + ri->merge_renames = NULL; + + if (!o->detect_rename) + return 1; + ri->head_renames = get_renames(o, head, common, head, merge, entries); ri->merge_renames = get_renames(o, merge, common, head, merge, entries); return process_renames(o, ri->head_renames, ri->merge_renames); @@ -1668,6 +1672,9 @@ static void cleanup_rename(struct string_list *rename) const struct rename *re; int i; + if (rename == NULL) + return; + for (i = 0; i < rename->nr; i++) { re = rename->items[i].util; diff_free_filepair(re->pair); -- 2.17.0.290.ge988e9ce2a
[PATCH v10 16/36] merge-recursive: split out code for determining diff_filepairs
Create a new function, get_diffpairs() to compute the diff_filepairs between two trees. While these are currently only used in get_renames(), I want them to be available to some new functions. No actual logic changes yet. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 84 ++- 1 file changed, 62 insertions(+), 22 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 5da60b9516..55a8ace948 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1322,24 +1322,15 @@ static int conflict_rename_rename_2to1(struct merge_options *o, } /* - * Get information of all renames which occurred between 'o_tree' and - * 'tree'. We need the three trees in the merge ('o_tree', 'a_tree' and - * 'b_tree') to be able to associate the correct cache entries with - * the rename information. 'tree' is always equal to either a_tree or b_tree. + * Get the diff_filepairs changed between o_tree and tree. */ -static struct string_list *get_renames(struct merge_options *o, - struct tree *tree, - struct tree *o_tree, - struct tree *a_tree, - struct tree *b_tree, - struct string_list *entries) +static struct diff_queue_struct *get_diffpairs(struct merge_options *o, + struct tree *o_tree, + struct tree *tree) { - int i; - struct string_list *renames; + struct diff_queue_struct *ret; struct diff_options opts; - renames = xcalloc(1, sizeof(struct string_list)); - diff_setup(&opts); opts.flags.recursive = 1; opts.flags.rename_empty = 0; @@ -1355,10 +1346,41 @@ static struct string_list *get_renames(struct merge_options *o, diffcore_std(&opts); if (opts.needed_rename_limit > o->needed_rename_limit) o->needed_rename_limit = opts.needed_rename_limit; - for (i = 0; i < diff_queued_diff.nr; ++i) { + + ret = xmalloc(sizeof(*ret)); + *ret = diff_queued_diff; + + opts.output_format = DIFF_FORMAT_NO_OUTPUT; + diff_queued_diff.nr = 0; + diff_queued_diff.queue = NULL; + diff_flush(&opts); + return ret; +} + +/* + * Get information of all renames which occurred in 'pairs', making use of + * any implicit directory renames inferred from the other side of history. + * We need the three trees in the merge ('o_tree', 'a_tree' and 'b_tree') + * to be able to associate the correct cache entries with the rename + * information; tree is always equal to either a_tree or b_tree. + */ +static struct string_list *get_renames(struct merge_options *o, + struct diff_queue_struct *pairs, + struct tree *tree, + struct tree *o_tree, + struct tree *a_tree, + struct tree *b_tree, + struct string_list *entries) +{ + int i; + struct string_list *renames; + + renames = xcalloc(1, sizeof(struct string_list)); + + for (i = 0; i < pairs->nr; ++i) { struct string_list_item *item; struct rename *re; - struct diff_filepair *pair = diff_queued_diff.queue[i]; + struct diff_filepair *pair = pairs->queue[i]; if (pair->status != 'R') { diff_free_filepair(pair); @@ -1383,9 +1405,6 @@ static struct string_list *get_renames(struct merge_options *o, item = string_list_insert(renames, pair->one->path); item->util = re; } - opts.output_format = DIFF_FORMAT_NO_OUTPUT; - diff_queued_diff.nr = 0; - diff_flush(&opts); return renames; } @@ -1656,15 +1675,36 @@ static int handle_renames(struct merge_options *o, struct string_list *entries, struct rename_info *ri) { + struct diff_queue_struct *head_pairs, *merge_pairs; + int clean; + ri->head_renames = NULL; ri->merge_renames = NULL; if (!o->detect_rename) return 1; - ri->head_renames = get_renames(o, head, common, head, merge, entries); - ri->merge_renames = get_renames(o, merge, common, head, merge, entries); - return process_renames(o, ri->head_renames, ri->merge_renames); + head_pairs = get_diffpairs(o, common, head); + merge_pairs = get_diffpairs(o, common, merge); + + ri->head_renames = get_renames(o, head_pairs, head, +common, head, merge, entries); + ri->merge_renames = get_renames(o, merge_pairs, me
[PATCH v10 28/36] merge-recursive: avoid spurious rename/rename conflict from dir renames
If a file on one side of history was renamed, and merely modified on the other side, then applying a directory rename to the modified side gives us a rename/rename(1to2) conflict. We should only apply directory renames to pairs representing either adds or renames. Making this change means that a directory rename testcase that was previously reported as a rename/delete conflict will now be reported as a modify/delete conflict. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 4 +-- t/t6043-merge-rename-directories.sh | 55 + 2 files changed, 27 insertions(+), 32 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 238711b038..27278d51bb 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1992,7 +1992,7 @@ static void compute_collisions(struct hashmap *collisions, char *new_path; struct diff_filepair *pair = pairs->queue[i]; - if (pair->status == 'D') + if (pair->status != 'A' && pair->status != 'R') continue; dir_rename_ent = check_dir_renamed(pair->two->path, dir_renames); @@ -2219,7 +2219,7 @@ static struct string_list *get_renames(struct merge_options *o, struct diff_filepair *pair = pairs->queue[i]; char *new_path; /* non-NULL only with directory renames */ - if (pair->status == 'D') { + if (pair->status != 'A' && pair->status != 'R') { diff_free_filepair(pair); continue; } diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index 5b84591445..45f620633f 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -2078,18 +2078,23 @@ test_expect_success '8b-check: Dual-directory rename, one into the others way, w ) ' -# Testcase 8c, rename+modify/delete -# (Related to testcases 5b and 8d) +# Testcase 8c, modify/delete or rename+modify/delete? +# (Related to testcases 5b, 8d, and 9h) # Commit O: z/{b,c,d} # Commit A: y/{b,c} # Commit B: z/{b,c,d_modified,e} -# Expected: y/{b,c,e}, CONFLICT(rename+modify/delete: x/d -> y/d or deleted) +# Expected: y/{b,c,e}, CONFLICT(modify/delete: on z/d) # -# Note: This testcase doesn't present any concerns for me...until you -# compare it with testcases 5b and 8d. See notes in 8d for more -# details. - -test_expect_success '8c-setup: rename+modify/delete' ' +# Note: It could easily be argued that the correct resolution here is +# y/{b,c,e}, CONFLICT(rename/delete: z/d -> y/d vs deleted) +# and that the modifed version of d should be present in y/ after +# the merge, just marked as conflicted. Indeed, I previously did +# argue that. But applying directory renames to the side of +# history where a file is merely modified results in spurious +# rename/rename(1to2) conflicts -- see testcase 9h. See also +# notes in 8d. + +test_expect_success '8c-setup: modify/delete or rename+modify/delete?' ' test_create_repo 8c && ( cd 8c && @@ -2122,32 +2127,32 @@ test_expect_success '8c-setup: rename+modify/delete' ' ) ' -test_expect_success '8c-check: rename+modify/delete' ' +test_expect_success '8c-check: modify/delete or rename+modify/delete' ' ( cd 8c && git checkout A^0 && test_must_fail git merge -s recursive B^0 >out && - test_i18ngrep "CONFLICT (rename/delete).* z/d.*y/d" out && + test_i18ngrep "CONFLICT (modify/delete).* z/d" out && git ls-files -s >out && - test_line_count = 4 out && + test_line_count = 5 out && git ls-files -u >out && - test_line_count = 1 out && + test_line_count = 2 out && git ls-files -o >out && test_line_count = 1 out && git rev-parse >actual \ - :0:y/b :0:y/c :0:y/e :3:y/d && + :0:y/b :0:y/c :0:y/e :1:z/d :3:z/d && git rev-parse >expect \ -O:z/b O:z/c B:z/e B:z/d && +O:z/b O:z/c B:z/e O:z/d B:z/d && test_cmp expect actual && - test_must_fail git rev-parse :1:y/d && - test_must_fail git rev-parse :2:y/d && - git ls-files -s y/d | grep ^100755 && - test_path_is_file y/d + test_must_fail git rev-parse :2:z/d && + git ls-files -s z/d | grep ^100755 && + test_path_is_file z/d && + test_path_is_missing y/d ) ' @@ -2161,16 +2166,6 @@ test_expect_success '8
[PATCH v10 30/36] merge-recursive: move more is_dirty handling to merge_content
conflict_rename_normal() was doing some handling for dirty files that more naturally belonged in merge_content. Move it, and rename a parameter for clarity while at it. Signed-off-by: Elijah Newren --- merge-recursive.c | 30 -- 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index b0f74cb243..7b0081565a 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2727,7 +2727,7 @@ static int handle_modify_delete(struct merge_options *o, static int merge_content(struct merge_options *o, const char *path, -int file_in_way, +int is_dirty, struct object_id *o_oid, int o_mode, struct object_id *a_oid, int a_mode, struct object_id *b_oid, int b_mode, @@ -2803,7 +2803,7 @@ static int merge_content(struct merge_options *o, return -1; } - if (df_conflict_remains || file_in_way) { + if (df_conflict_remains || is_dirty) { char *new_path; if (o->call_depth) { remove_file_from_cache(path); @@ -2825,6 +2825,10 @@ static int merge_content(struct merge_options *o, } new_path = unique_path(o, path, rename_conflict_info->branch1); + if (is_dirty) { + output(o, 1, _("Refusing to lose dirty file at %s"), + path); + } output(o, 1, _("Adding as %s instead"), new_path); if (update_file(o, 0, &mfi.oid, mfi.mode, new_path)) { free(new_path); @@ -2834,7 +2838,7 @@ static int merge_content(struct merge_options *o, mfi.clean = 0; } else if (update_file(o, mfi.clean, &mfi.oid, mfi.mode, path)) return -1; - return mfi.clean; + return !is_dirty && mfi.clean; } static int conflict_rename_normal(struct merge_options *o, @@ -2844,21 +2848,10 @@ static int conflict_rename_normal(struct merge_options *o, struct object_id *b_oid, unsigned int b_mode, struct rename_conflict_info *ci) { - int clean_merge; - int file_in_the_way = 0; - - if (was_dirty(o, path)) { - file_in_the_way = 1; - output(o, 1, _("Refusing to lose dirty file at %s"), path); - } - /* Merge the content and write it out */ - clean_merge = merge_content(o, path, file_in_the_way, - o_oid, o_mode, a_oid, a_mode, b_oid, b_mode, - ci); - if (clean_merge > 0 && file_in_the_way) - clean_merge = 0; - return clean_merge; + return merge_content(o, path, was_dirty(o, path), +o_oid, o_mode, a_oid, a_mode, b_oid, b_mode, +ci); } /* Per entry merge function */ @@ -2981,7 +2974,8 @@ static int process_entry(struct merge_options *o, } else if (a_oid && b_oid) { /* Case C: Added in both (check for same permissions) and */ /* case D: Modified in both, but differently. */ - clean_merge = merge_content(o, path, 0 /* file_in_way */, + int is_dirty = 0; /* unpack_trees would have bailed if dirty */ + clean_merge = merge_content(o, path, is_dirty, o_oid, o_mode, a_oid, a_mode, b_oid, b_mode, NULL); } else if (!o_oid && !a_oid && !b_oid) { -- 2.17.0.290.ge988e9ce2a
[PATCH v10 23/36] merge-recursive: apply necessary modifications for directory renames
This commit hooks together all the directory rename logic by making the necessary changes to the rename struct, it's dst_entry, and the diff_filepair under consideration. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 187 +++- t/t6043-merge-rename-directories.sh | 50 2 files changed, 211 insertions(+), 26 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index d569e3e893..ec0bbcc3f4 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -180,6 +180,7 @@ static int oid_eq(const struct object_id *a, const struct object_id *b) enum rename_type { RENAME_NORMAL = 0, + RENAME_DIR, RENAME_DELETE, RENAME_ONE_FILE_TO_ONE, RENAME_ONE_FILE_TO_TWO, @@ -610,6 +611,7 @@ struct rename { */ struct stage_data *src_entry; struct stage_data *dst_entry; + unsigned add_turned_into_rename:1; unsigned processed:1; }; @@ -644,6 +646,27 @@ static int update_stages(struct merge_options *opt, const char *path, return 0; } +static int update_stages_for_stage_data(struct merge_options *opt, + const char *path, + const struct stage_data *stage_data) +{ + struct diff_filespec o, a, b; + + o.mode = stage_data->stages[1].mode; + oidcpy(&o.oid, &stage_data->stages[1].oid); + + a.mode = stage_data->stages[2].mode; + oidcpy(&a.oid, &stage_data->stages[2].oid); + + b.mode = stage_data->stages[3].mode; + oidcpy(&b.oid, &stage_data->stages[3].oid); + + return update_stages(opt, path, +is_null_oid(&o.oid) ? NULL : &o, +is_null_oid(&a.oid) ? NULL : &a, +is_null_oid(&b.oid) ? NULL : &b); +} + static void update_entry(struct stage_data *entry, struct diff_filespec *o, struct diff_filespec *a, @@ -1121,6 +1144,18 @@ static int merge_file_one(struct merge_options *o, return merge_file_1(o, &one, &a, &b, branch1, branch2, mfi); } +static int conflict_rename_dir(struct merge_options *o, + struct diff_filepair *pair, + const char *rename_branch, + const char *other_branch) +{ + const struct diff_filespec *dest = pair->two; + + if (update_file(o, 1, &dest->oid, dest->mode, dest->path)) + return -1; + return 0; +} + static int handle_change_delete(struct merge_options *o, const char *path, const char *old_path, const struct object_id *o_oid, int o_mode, @@ -1390,6 +1425,24 @@ static int conflict_rename_rename_2to1(struct merge_options *o, if (!ret) ret = update_file(o, 0, &mfi_c2.oid, mfi_c2.mode, new_path2); + /* +* unpack_trees() actually populates the index for us for +* "normal" rename/rename(2to1) situtations so that the +* correct entries are at the higher stages, which would +* make the call below to update_stages_for_stage_data +* unnecessary. However, if either of the renames came +* from a directory rename, then unpack_trees() will not +* have gotten the right data loaded into the index, so we +* need to do so now. (While it'd be tempting to move this +* call to update_stages_for_stage_data() to +* apply_directory_rename_modifications(), that would break +* our intermediate calls to would_lose_untracked() since +* those rely on the current in-memory index. See also the +* big "NOTE" in update_stages()). +*/ + if (update_stages_for_stage_data(o, path, ci->dst_entry1)) + ret = -1; + free(new_path2); free(new_path1); } @@ -1952,6 +2005,111 @@ static char *check_for_directory_rename(struct merge_options *o, return new_path; } +static void apply_directory_rename_modifications(struct merge_options *o, +struct diff_filepair *pair, +char *new_path, +struct rename *re, +struct tree *tree, +struct tree *o_tree, +struct tree *a_tree, +struct tree *b_tree, +struct string_list *entries, +
[PATCH v10 20/36] merge-recursive: add computation of collisions due to dir rename & merging
directory renaming and merging can cause one or more files to be moved to where an existing file is, or to cause several files to all be moved to the same (otherwise vacant) location. Add checking and reporting for such cases, falling back to no-directory-rename handling for such paths. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 146 +- merge-recursive.h | 7 +++ 2 files changed, 150 insertions(+), 3 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 3b0e509513..25ea6841fc 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -87,6 +87,29 @@ static void dir_rename_entry_init(struct dir_rename_entry *entry, string_list_init(&entry->possible_new_dirs, 0); } +static struct collision_entry *collision_find_entry(struct hashmap *hashmap, + char *target_file) +{ + struct collision_entry key; + + hashmap_entry_init(&key, strhash(target_file)); + key.target_file = target_file; + return hashmap_get(hashmap, &key, NULL); +} + +static int collision_cmp(void *unused_cmp_data, +const struct collision_entry *e1, +const struct collision_entry *e2, +const void *unused_keydata) +{ + return strcmp(e1->target_file, e2->target_file); +} + +static void collision_init(struct hashmap *map) +{ + hashmap_init(map, (hashmap_cmp_fn) collision_cmp, NULL, 0); +} + static void flush_output(struct merge_options *o) { if (o->buffer_output < 2 && o->obuf.len) { @@ -1404,6 +1427,31 @@ static int tree_has_path(struct tree *tree, const char *path) &hashy, &mode_o); } +/* + * Return a new string that replaces the beginning portion (which matches + * entry->dir), with entry->new_dir. In perl-speak: + * new_path_name = (old_path =~ s/entry->dir/entry->new_dir/); + * NOTE: + * Caller must ensure that old_path starts with entry->dir + '/'. + */ +static char *apply_dir_rename(struct dir_rename_entry *entry, + const char *old_path) +{ + struct strbuf new_path = STRBUF_INIT; + int oldlen, newlen; + + if (entry->non_unique_new_dir) + return NULL; + + oldlen = strlen(entry->dir); + newlen = entry->new_dir.len + (strlen(old_path) - oldlen) + 1; + strbuf_grow(&new_path, newlen); + strbuf_addbuf(&new_path, &entry->new_dir); + strbuf_addstr(&new_path, &old_path[oldlen]); + + return strbuf_detach(&new_path, NULL); +} + static void get_renamed_dir_portion(const char *old_path, const char *new_path, char **old_dir, char **new_dir) { @@ -1673,6 +1721,84 @@ static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs, return dir_renames; } +static struct dir_rename_entry *check_dir_renamed(const char *path, + struct hashmap *dir_renames) +{ + char temp[PATH_MAX]; + char *end; + struct dir_rename_entry *entry; + + strcpy(temp, path); + while ((end = strrchr(temp, '/'))) { + *end = '\0'; + entry = dir_rename_find_entry(dir_renames, temp); + if (entry) + return entry; + } + return NULL; +} + +static void compute_collisions(struct hashmap *collisions, + struct hashmap *dir_renames, + struct diff_queue_struct *pairs) +{ + int i; + + /* +* Multiple files can be mapped to the same path due to directory +* renames done by the other side of history. Since that other +* side of history could have merged multiple directories into one, +* if our side of history added the same file basename to each of +* those directories, then all N of them would get implicitly +* renamed by the directory rename detection into the same path, +* and we'd get an add/add/.../add conflict, and all those adds +* from *this* side of history. This is not representable in the +* index, and users aren't going to easily be able to make sense of +* it. So we need to provide a good warning about what's +* happening, and fall back to no-directory-rename detection +* behavior for those paths. +* +* See testcases 9e and all of section 5 from t6043 for examples. +*/ + collision_init(collisions); + + for (i = 0; i < pairs->nr; ++i) { + struct dir_rename_entry *dir_rename_ent; + struct collision_entry *collision_ent; + char *new_path; + struct diff_filepair *pair = pairs->queue[i]; + + if (pair->status == 'D') + continue; + dir_rename_e
[PATCH v10 31/36] merge-recursive: avoid triggering add_cacheinfo error with dirty mod
If a cherry-pick or merge with a rename results in a skippable update (due to the merged content matching what HEAD already had), but the working directory is dirty, avoid trying to refresh the index as that will fail. Signed-off-by: Elijah Newren --- merge-recursive.c | 2 +- t/t3501-revert-cherry-pick.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 7b0081565a..b32e8d817a 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2786,7 +2786,7 @@ static int merge_content(struct merge_options *o, path_renamed_outside_HEAD = !path2 || !strcmp(path, path2); if (!path_renamed_outside_HEAD) { if (add_cacheinfo(o, mfi.mode, &mfi.oid, path, - 0, (!o->call_depth), 0)) + 0, (!o->call_depth && !is_dirty), 0)) return -1; return mfi.clean; } diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index 3871807d09..d1c68af8c5 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' ' test_cmp expect actual ' -test_expect_failure 'cherry-pick works with dirty renamed file' ' +test_expect_success 'cherry-pick works with dirty renamed file' ' test_commit to-rename && git checkout -b unrelated && test_commit unrelated && -- 2.17.0.290.ge988e9ce2a
[PATCH v10 35/36] merge-recursive: make "Auto-merging" comment show for other merges
Previously, merge_content() would print "Auto-merging" whenever the final content and mode aren't already available from HEAD. There are a few problems with this: 1) There are other code paths doing merges that should probably have the same message printed, in particular rename/rename(2to1) which cannot call into the normal rename logic. 2) If both sides of the merge have modifications, then a content merge is needed. It may turn out that the end result matches one of the sides (because the other only had a subset of the same changes), but the merge was still needed. Currently, the message will not print in that case, though it seems like it should. Move the printing of this message to merge_file_1() in order to address both issues. Signed-off-by: Elijah Newren --- Part of the size of the diff was due to fixing the alignment of function arguments while I was adding another argument to the list... merge-recursive.c | 65 --- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 1a481fa3dc..212d34d268 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1063,12 +1063,13 @@ static int merge_3way(struct merge_options *o, } static int merge_file_1(struct merge_options *o, - const struct diff_filespec *one, - const struct diff_filespec *a, - const struct diff_filespec *b, - const char *branch1, - const char *branch2, - struct merge_file_info *result) + const struct diff_filespec *one, + const struct diff_filespec *a, + const struct diff_filespec *b, + const char *filename, + const char *branch1, + const char *branch2, + struct merge_file_info *result) { result->merge = 0; result->clean = 1; @@ -1148,18 +1149,22 @@ static int merge_file_1(struct merge_options *o, die("BUG: unsupported object type in the tree"); } + if (result->merge) + output(o, 2, _("Auto-merging %s"), filename); + return 0; } static int merge_file_special_markers(struct merge_options *o, - const struct diff_filespec *one, - const struct diff_filespec *a, - const struct diff_filespec *b, - const char *branch1, - const char *filename1, - const char *branch2, - const char *filename2, - struct merge_file_info *mfi) + const struct diff_filespec *one, + const struct diff_filespec *a, + const struct diff_filespec *b, + const char *target_filename, + const char *branch1, + const char *filename1, + const char *branch2, + const char *filename2, + struct merge_file_info *mfi) { char *side1 = NULL; char *side2 = NULL; @@ -1170,22 +1175,23 @@ static int merge_file_special_markers(struct merge_options *o, if (filename2) side2 = xstrfmt("%s:%s", branch2, filename2); - ret = merge_file_1(o, one, a, b, + ret = merge_file_1(o, one, a, b, target_filename, side1 ? side1 : branch1, side2 ? side2 : branch2, mfi); + free(side1); free(side2); return ret; } static int merge_file_one(struct merge_options *o, -const char *path, -const struct object_id *o_oid, int o_mode, -const struct object_id *a_oid, int a_mode, -const struct object_id *b_oid, int b_mode, -const char *branch1, -const char *branch2, -struct merge_file_info *mfi) + const char *path, + const struct object_id *o_oid, int o_mode, + const struct object_id *a_oid, int a_mode, + const struct object_id *b_oid, int b_mode, + const char *branch1, + const char *branch2, + struct merge_file_info *mfi
[PATCH v10 32/36] t6046: testcases checking whether updates can be skipped in a merge
Add several tests checking whether updates can be skipped in a merge. Also add several similar testcases for where updates cannot be skipped in a merge to make sure that we skip if and only if we should. In particular: * Testcase 1a (particularly 1a-check-L) would have pointed out the problem Linus has been dealing with for year with his merges[1]. * Testcase 2a (particularly 2a-check-L) would have pointed out the problem with my directory-rename-series before it broke master[2]. * Testcases 3[ab] (particularly 3a-check-L) provide a simpler testcase than 12b of t6043 making that one easier to understand. * There are several complementary testcases to make sure we're not just fixing those particular issues while regressing in the opposite direction. * There are also a pair of tests for the special case when a merge results in a skippable update AND the user has dirty modifications to the path. [1] https://public-inbox.org/git/CA+55aFzLZ3UkG5svqZwSnhNk75=fxjrkvu1m_rhbg54nooa...@mail.gmail.com/ [2] https://public-inbox.org/git/xmqqmuya43cs@gitster-ct.c.googlers.com/ Signed-off-by: Elijah Newren --- Stefan Beller reviewed an RFC version of this patch and gave his Reviewed-by for it, but I've significantly modified it since then, including: - new tests for dirty files being present - new test for a carefully crafted rename/add conflict (which I constructed to try see if there was still a way to get mis-merges with the RFC patches) - better test cleanup/recovery (add git clean to go with git reset) - added modification timestamp checking to relevant tests to verify that when merge-recursive claims certain file updates were skipped during the merge that they really were skipped. t/t6046-merge-skip-unneeded-updates.sh | 761 + 1 file changed, 761 insertions(+) create mode 100755 t/t6046-merge-skip-unneeded-updates.sh diff --git a/t/t6046-merge-skip-unneeded-updates.sh b/t/t6046-merge-skip-unneeded-updates.sh new file mode 100755 index 00..911e2f87a4 --- /dev/null +++ b/t/t6046-merge-skip-unneeded-updates.sh @@ -0,0 +1,761 @@ +#!/bin/sh + +test_description="merge cases" + +# The setup for all of them, pictorially, is: +# +# A +# o +# / \ +# O o ? +# \ / +# o +# B +# +# To help make it easier to follow the flow of tests, they have been +# divided into sections and each test will start with a quick explanation +# of what commits O, A, and B contain. +# +# Notation: +#z/{b,c} means files z/b and z/c both exist +#x/d_1 means file x/d exists with content d1. (Purpose of the +# underscore notation is to differentiate different +# files that might be renamed into each other's paths.) + +. ./test-lib.sh + + +### +# SECTION 1: Cases involving no renames (one side has subset of changes of +#the other side) +### + +# Testcase 1a, Changes on A, subset of changes on B +# Commit O: b_1 +# Commit A: b_2 +# Commit B: b_3 +# Expected: b_2 + +test_expect_success '1a-setup: Modify(A)/Modify(B), change on B subset of A' ' + test_create_repo 1a && + ( + cd 1a && + + test_write_lines 1 2 3 4 5 6 7 8 9 10 >b + git add b && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + test_write_lines 1 2 3 4 5 5.5 6 7 8 9 10 10.5 >b && + git add b && + test_tick && + git commit -m "A" && + + git checkout B && + test_write_lines 1 2 3 4 5 5.5 6 7 8 9 10 >b && + git add b && + test_tick && + git commit -m "B" + ) +' + +test_expect_failure '1a-check-L: Modify(A)/Modify(B), change on B subset of A' ' + test_when_finished "git -C 1a reset --hard" && + test_when_finished "git -C 1a clean -fd" && + ( + cd 1a && + + git checkout A^0 && + + test-tool chmtime =31337 b && + test-tool chmtime -v +0 b >expected-mtime && + + GIT_MERGE_VERBOSITY=3 git merge -s recursive B^0 >out 2>err && + + test_i18ngrep "Skipped b" out && + test_must_be_empty err && + + test-tool chmtime -v +0 b >actual-mtime && + test_cmp expected-mtime actual-mtime && + + git ls-files -s >index_files && + test_line_count = 1 index_files && + + git rev-parse >actual HEAD:b && + git rev-parse >expect A:b && + test_cmp expect actual && + + git hash-object b >actual && +
[PATCH v10 00/36] Add directory rename detection to git
This series is a reboot of the directory rename detection series that was merged to master and then reverted due to the final patch having a buggy can-skip-update check, as noted at https://public-inbox.org/git/xmqqmuya43cs@gitster-ct.c.googlers.com/ This series based on top of master. This updated series fixes the problem found with the previous series, and also fixes Linus' issue with unnecessary rebuilds noted at https://public-inbox.org/git/CA+55aFzLZ3UkG5svqZwSnhNk75=fxjrkvu1m_rhbg54nooa...@mail.gmail.com/ For the original details about design considerations surrounding directory rename detection, see https://public-inbox.org/git/20171110190550.27059-1-new...@gmail.com/ Patches 1--28 are identical to what was previously merged to master, modulo trivial compilation fixes due to the fact that I've rebased on master which now includes commit 916bc35b29af ("tree-walk: convert tree entry functions to object_id", 2018-03-12). As such, I've retained the Reviewed-by and Signed-off-by tags for these first 28 patches. (The final patch of the original series, patch 29, has been rewritten and replaced in this series.) The remaining eight patches are new; a brief summary: merge-recursive: improve add_cacheinfo error handling merge-recursive: move more is_dirty handling to merge_content merge-recursive: avoid triggering add_cacheinfo error with dirty mod When Junio was bit by the previous series, the code reached a detected error state that should not ever be hit in production. That was bad enough, but the problem compounded because the code simply printed a vague not-very-scary-sounding error, and returned an error code that the caller ignored (which not only proceeded to then handle other paths which might print messages causing the error to scroll off the screen, but could result in a "clean" merge). Fix issues with the error handling...and then deal with the breakage of one particular test that was triggering this codepath. t6046: testcases checking whether updates can be skipped in a merge Add a fairly comprehensive set of tests for the skipability of working tree updates. merge-recursive: fix was_tracked() to quit lying with some renamed paths merge-recursive: fix remainder of was_dirty() to use original index Instead of using the current index as a (rather imperfect) proxy for the state of the index just before the merge, keep a copy of the original index around so we can get correct answers to whether certain paths were tracked or dirty before the merge. merge-recursive: make "Auto-merging" comment show for other merges merge-recursive: fix check for skipability of working tree updates Fix and simplify the skipability check. Due to some tests being picky about output, the first of these two patches exists to avoid triggering the "Auto-merging $FILE" message too often with the simplified logic; in the process, it fixes a pair of existing issues with when those messages are shown, making it more accurate in general. Additional testing: * I've re-merged all ~13k merge commits in git.git with both git-2.17.0 and this version of git, comparing the results to each other in detail. (Including stdout & stderr, as well as the output of subsequent commands like `git status`, `git ls-files -s`, `git diff -M`, `git diff -M --staged`). The only differences were in 23 merges of either git-gui or gitk which involved directory renames (e.g. git-2.17.0's merge would result in files like 'lib/tools.tcl' or 'po/ru.po' instead of the expected 'git-gui/lib/tools.tcl' or 'gitk-git/po/ru.po') * I'm trying to do the same with linux.git, but it looks like that will take nearly a week to complete... My biggest question: * Is there any other testing others would like to see, in order to avoid a repeat of the pain from my previous series and allow us to safely merge this newer one? Elijah Newren (36): directory rename detection: basic testcases directory rename detection: directory splitting testcases directory rename detection: testcases to avoid taking detection too far directory rename detection: partially renamed directory testcase/discussion directory rename detection: files/directories in the way of some renames directory rename detection: testcases checking which side did the rename directory rename detection: more involved edge/corner testcases directory rename detection: testcases exploring possibly suboptimal merges directory rename detection: miscellaneous testcases to complete coverage directory rename detection: tests for handling overwriting untracked files directory rename detection: tests for handling overwriting dirty files merge-recursive: move the get_renames() function merge-recursive: introduce new functions to handle rename logic merge-recursive: fix leaks of allocated ren
[PATCH v10 14/36] merge-recursive: fix leaks of allocated renames and diff_filepairs
get_renames() has always zero'ed out diff_queued_diff.nr while only manually free'ing diff_filepairs that did not correspond to renames. Further, it allocated struct renames that were tucked away in the return string_list. Make sure all of these are deallocated when we are done with them. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 20 +++- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 40e142efdb..fc96653f63 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1663,13 +1663,23 @@ static int handle_renames(struct merge_options *o, return process_renames(o, ri->head_renames, ri->merge_renames); } -static void cleanup_renames(struct rename_info *re_info) +static void cleanup_rename(struct string_list *rename) { - string_list_clear(re_info->head_renames, 0); - string_list_clear(re_info->merge_renames, 0); + const struct rename *re; + int i; - free(re_info->head_renames); - free(re_info->merge_renames); + for (i = 0; i < rename->nr; i++) { + re = rename->items[i].util; + diff_free_filepair(re->pair); + } + string_list_clear(rename, 1); + free(rename); +} + +static void cleanup_renames(struct rename_info *re_info) +{ + cleanup_rename(re_info->head_renames); + cleanup_rename(re_info->merge_renames); } static struct object_id *stage_oid(const struct object_id *oid, unsigned mode) -- 2.17.0.290.ge988e9ce2a
[PATCH v10 26/36] merge-recursive: fix remaining directory rename + dirty overwrite cases
Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 25 ++--- t/t6043-merge-rename-directories.sh | 8 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 7fdcba4f22..238711b038 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1324,11 +1324,22 @@ static int handle_file(struct merge_options *o, add = filespec_from_entry(&other, dst_entry, stage ^ 1); if (add) { + int ren_src_was_dirty = was_dirty(o, rename->path); char *add_name = unique_path(o, rename->path, other_branch); if (update_file(o, 0, &add->oid, add->mode, add_name)) return -1; - remove_file(o, 0, rename->path, 0); + if (ren_src_was_dirty) { + output(o, 1, _("Refusing to lose dirty file at %s"), + rename->path); + } + /* +* Because the double negatives somehow keep confusing me... +*1) update_wd iff !ren_src_was_dirty. +*2) no_wd iff !update_wd +*3) so, no_wd == !!ren_src_was_dirty == ren_src_was_dirty +*/ + remove_file(o, 0, rename->path, ren_src_was_dirty); dst_name = unique_path(o, rename->path, cur_branch); } else { if (dir_in_way(rename->path, !o->call_depth, 0)) { @@ -1466,7 +1477,10 @@ static int conflict_rename_rename_2to1(struct merge_options *o, char *new_path2 = unique_path(o, path, ci->branch2); output(o, 1, _("Renaming %s to %s and %s to %s instead"), a->path, new_path1, b->path, new_path2); - if (would_lose_untracked(path)) + if (was_dirty(o, path)) + output(o, 1, _("Refusing to lose dirty file at %s"), + path); + else if (would_lose_untracked(path)) /* * Only way we get here is if both renames were from * a directory rename AND user had an untracked file @@ -2075,6 +2089,7 @@ static void apply_directory_rename_modifications(struct merge_options *o, { struct string_list_item *item; int stage = (tree == a_tree ? 2 : 3); + int update_wd; /* * In all cases where we can do directory rename detection, @@ -2085,7 +2100,11 @@ static void apply_directory_rename_modifications(struct merge_options *o, * saying the file would have been overwritten), but it might * be dirty, though. */ - remove_file(o, 1, pair->two->path, 0 /* no_wd */); + update_wd = !was_dirty(o, pair->two->path); + if (!update_wd) + output(o, 1, _("Refusing to lose dirty file at %s"), + pair->two->path); + remove_file(o, 1, pair->two->path, !update_wd); /* Find or create a new re->dst_entry */ item = string_list_lookup(entries, new_path); diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index b94ba066fe..33e2649824 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -3370,7 +3370,7 @@ test_expect_success '11b-setup: Avoid losing dirty file involved in directory re ) ' -test_expect_failure '11b-check: Avoid losing dirty file involved in directory rename' ' +test_expect_success '11b-check: Avoid losing dirty file involved in directory rename' ' ( cd 11b && @@ -3512,7 +3512,7 @@ test_expect_success '11d-setup: Avoid losing not-uptodate with rename + D/F conf ) ' -test_expect_failure '11d-check: Avoid losing not-uptodate with rename + D/F conflict' ' +test_expect_success '11d-check: Avoid losing not-uptodate with rename + D/F conflict' ' ( cd 11d && @@ -3591,7 +3591,7 @@ test_expect_success '11e-setup: Avoid deleting not-uptodate with dir rename/rena ) ' -test_expect_failure '11e-check: Avoid deleting not-uptodate with dir rename/rename(1to2)/add' ' +test_expect_success '11e-check: Avoid deleting not-uptodate with dir rename/rename(1to2)/add' ' ( cd 11e && @@ -3667,7 +3667,7 @@ test_expect_success '11f-setup: Avoid deleting not-uptodate with dir rename/rena ) ' -test_expect_failure '11f-check: Avoid deleting not-uptodate with dir rename/rename(2to1)' ' +test_expect_success '11f-check: Avoid deleting not-uptodate with dir rename/rename(2to1)' ' ( cd 11f && -- 2.17.0.290.ge988e9ce2a
[PATCH v10 07/36] directory rename detection: more involved edge/corner testcases
Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6043-merge-rename-directories.sh | 396 1 file changed, 396 insertions(+) diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index 9ae245a522..6db1439675 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -1516,4 +1516,400 @@ test_expect_success '6e-check: Add/add from one side' ' # side of history is the one doing the renaming. ### + +### +# SECTION 7: More involved Edge/Corner cases +# +# The ruleset we have generated in the above sections seems to provide +# well-defined merges. But can we find edge/corner cases that either (a) +# are harder for users to understand, or (b) have a resolution that is +# non-intuitive or suboptimal? +# +# The testcases in this section dive into cases that I've tried to craft in +# a way to find some that might be surprising to users or difficult for +# them to understand (the next section will look at non-intuitive or +# suboptimal merge results). Some of the testcases are similar to ones +# from past sections, but have been simplified to try to highlight error +# messages using a "modified" path (due to the directory rename). Are +# users okay with these? +# +# In my opinion, testcases that are difficult to understand from this +# section is due to difficulty in the testcase rather than the directory +# renaming (similar to how t6042 and t6036 have difficult resolutions due +# to the problem setup itself being complex). And I don't think the +# error messages are a problem. +# +# On the other hand, the testcases in section 8 worry me slightly more... +### + +# Testcase 7a, rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file +# Commit O: z/{b,c} +# Commit A: y/{b,c} +# Commit B: w/b, x/c, z/d +# Expected: y/d, CONFLICT(rename/rename for both z/b and z/c) +# NOTE: There's a rename of z/ here, y/ has more renames, so z/d -> y/d. + +test_expect_success '7a-setup: rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file' ' + test_create_repo 7a && + ( + cd 7a && + + mkdir z && + echo b >z/b && + echo c >z/c && + git add z && + test_tick && + git commit -m "O" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + git mv z y && + test_tick && + git commit -m "A" && + + git checkout B && + mkdir w && + mkdir x && + git mv z/b w/ && + git mv z/c x/ && + echo d > z/d && + git add z/d && + test_tick && + git commit -m "B" + ) +' + +test_expect_failure '7a-check: rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file' ' + ( + cd 7a && + + git checkout A^0 && + + test_must_fail git merge -s recursive B^0 >out && + test_i18ngrep "CONFLICT (rename/rename).*z/b.*y/b.*w/b" out && + test_i18ngrep "CONFLICT (rename/rename).*z/c.*y/c.*x/c" out && + + git ls-files -s >out && + test_line_count = 7 out && + git ls-files -u >out && + test_line_count = 6 out && + git ls-files -o >out && + test_line_count = 1 out && + + git rev-parse >actual \ + :1:z/b :2:y/b :3:w/b :1:z/c :2:y/c :3:x/c :0:y/d && + git rev-parse >expect \ +O:z/b O:z/b O:z/b O:z/c O:z/c O:z/c B:z/d && + test_cmp expect actual && + + git hash-object >actual \ + y/b w/b y/c x/c && + git rev-parse >expect \ + O:z/b O:z/b O:z/c O:z/c && + test_cmp expect actual + ) +' + +# Testcase 7b, rename/rename(2to1), but only due to transitive rename +# (Related to testcase 1d) +# Commit O: z/{b,c}, x/d_1, w/d_2 +# Commit A: y/{b,c,d_2}, x/d_1 +# Commit B: z/{b,c,d_1},w/d_2 +# Expected: y/{b,c}, CONFLICT(rename/rename(2to1): x/d_1, w/d_2 -> y_d) + +test_expect_success '7b-setup: rename/rename(2to1), but only due to transitive rename' ' + test_create_repo 7b && + ( + cd 7b && + + mkdir z && + mkdir x && + mkdir w && + echo b >z/b && + echo c >z/c && + echo d1 > x/d && + echo d2 > w/d && + git add z x w && +
[PATCH v10 36/36] merge-recursive: fix check for skipability of working tree updates
The can-working-tree-updates-be-skipped check has had a long and blemished history. The update can be skipped iff: a) The merge is clean b) The merge matches what was in HEAD (content, mode, pathname) c) The target path is usable (i.e. not involved in D/F conflict) Traditionally, we split b into parts: b1) The merged result matches the content and mode found in HEAD b2) The merged target path existed in HEAD Steps a & b1 are easy to check; we have always gotten those right. While it is easy to overlook step c, this was fixed seven years ago with commit 4ab9a157d069 ("merge_content(): Check whether D/F conflicts are still present", 2010-09-20). merge-recursive didn't have a readily available way to directly check step b2, so various approximations were used: * In commit b2c8c0a76274 ("merge-recursive: When we detect we can skip an update, actually skip it", 2011-02-28), it was noted that although the code claimed it was skipping the update, it did not actually skip the update. The code was made to skip it, but used lstat(path, ...) as an approximation to path-was-tracked-in-index-before-merge. * In commit 5b448b853030 ("merge-recursive: When we detect we can skip an update, actually skip it", 2011-08-11), the problem with using lstat was noted. It was changed to the approximation path2 && strcmp(path, path2) which is also wrong. !path2 || strcmp(path, path2) would have been better, but would have fallen short with directory renames. * In c5b761fb2711 ("merge-recursive: ensure we write updates for directory-renamed file", 2018-02-14), the problem with the previous approximation was noted and changed to was_tracked(path) That looks close to what we were trying to answer, but was_tracked() as implemented at the time should have been named is_tracked(); it returned something different than what we were looking for. * To make matters more complex, fixing was_tracked() isn't sufficient because the splitting of b into b1 and b2 is wrong. Consider the following merge with a rename/add conflict: side A: modify foo, add unrelated bar side B: rename foo->bar (but don't modify the mode or contents) In this case, the three-way merge of original foo, A's foo, and B's bar will result in a desired pathname of bar with the same mode/contents that A had for foo. Thus, A had the right mode and contents for the file, and it had the right pathname present (namely, bar), but the bar that was present was unrelated to the contents, so the working tree update was not skippable. Fix this by introducing a new function: was_tracked_and_matches(o, path, &mfi.oid, mfi.mode) and use it to directly check for condition b. Signed-off-by: Elijah Newren --- merge-recursive.c | 48 +- t/t6022-merge-rename.sh| 2 +- t/t6043-merge-rename-directories.sh| 2 +- t/t6046-merge-skip-unneeded-updates.sh | 10 +++--- 4 files changed, 39 insertions(+), 23 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 212d34d268..1de8dc1c53 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -779,6 +779,25 @@ static int dir_in_way(const char *path, int check_working_copy, int empty_ok) !(empty_ok && is_empty_dir(path)); } +/* + * Returns whether path was tracked in the index before the merge started, + * and its oid and mode match the specified values + */ +static int was_tracked_and_matches(struct merge_options *o, const char *path, + const struct object_id *oid, unsigned mode) +{ + int pos = index_name_pos(&o->orig_index, path, strlen(path)); + struct cache_entry *ce; + + if (0 > pos) + /* we were not tracking this path before the merge */ + return 0; + + /* See if the file we were tracking before matches */ + ce = o->orig_index.cache[pos]; + return (oid_eq(&ce->oid, oid) && ce->ce_mode == mode); +} + /* * Returns whether path was tracked in the index before the merge started */ @@ -2821,23 +2840,20 @@ static int merge_content(struct merge_options *o, o->branch2, path2, &mfi)) return -1; - if (mfi.clean && !df_conflict_remains && - oid_eq(&mfi.oid, a_oid) && mfi.mode == a_mode) { - int path_renamed_outside_HEAD; + /* +* We can skip updating the working tree file iff: +* a) The merge is clean +* b) The merge matches what was in HEAD (content, mode, pathname) +* c) The target path is usable (i.e. not involved in D/F conflict) +*/ + if (mfi.clean && + was_tracked_and_matches(o, path, &mfi.oid, mfi.mode) && + !df_conflict_remains) { output(o, 3, _("Skipped %s (merged same as existing)"), path); - /* -*
[PATCH v10 19/36] merge-recursive: check for directory level conflicts
Before trying to apply directory renames to paths within the given directories, we want to make sure that there aren't conflicts at the directory level. There will be additional checks at the individual file level too, which will be added later. Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 119 ++ 1 file changed, 119 insertions(+) diff --git a/merge-recursive.c b/merge-recursive.c index 22c5e8e5c9..3b0e509513 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1395,6 +1395,15 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *o, return ret; } +static int tree_has_path(struct tree *tree, const char *path) +{ + struct object_id hashy; + unsigned int mode_o; + + return !get_tree_entry(&tree->object.oid, path, + &hashy, &mode_o); +} + static void get_renamed_dir_portion(const char *old_path, const char *new_path, char **old_dir, char **new_dir) { @@ -1450,6 +1459,112 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path, } } +static void remove_hashmap_entries(struct hashmap *dir_renames, + struct string_list *items_to_remove) +{ + int i; + struct dir_rename_entry *entry; + + for (i = 0; i < items_to_remove->nr; i++) { + entry = items_to_remove->items[i].util; + hashmap_remove(dir_renames, entry, NULL); + } + string_list_clear(items_to_remove, 0); +} + +/* + * There are a couple things we want to do at the directory level: + * 1. Check for both sides renaming to the same thing, in order to avoid + * implicit renaming of files that should be left in place. (See + * testcase 6b in t6043 for details.) + * 2. Prune directory renames if there are still files left in the + * the original directory. These represent a partial directory rename, + * i.e. a rename where only some of the files within the directory + * were renamed elsewhere. (Technically, this could be done earlier + * in get_directory_renames(), except that would prevent us from + * doing the previous check and thus failing testcase 6b.) + * 3. Check for rename/rename(1to2) conflicts (at the directory level). + * In the future, we could potentially record this info as well and + * omit reporting rename/rename(1to2) conflicts for each path within + * the affected directories, thus cleaning up the merge output. + * NOTE: We do NOT check for rename/rename(2to1) conflicts at the + * directory level, because merging directories is fine. If it + * causes conflicts for files within those merged directories, then + * that should be detected at the individual path level. + */ +static void handle_directory_level_conflicts(struct merge_options *o, +struct hashmap *dir_re_head, +struct tree *head, +struct hashmap *dir_re_merge, +struct tree *merge) +{ + struct hashmap_iter iter; + struct dir_rename_entry *head_ent; + struct dir_rename_entry *merge_ent; + + struct string_list remove_from_head = STRING_LIST_INIT_NODUP; + struct string_list remove_from_merge = STRING_LIST_INIT_NODUP; + + hashmap_iter_init(dir_re_head, &iter); + while ((head_ent = hashmap_iter_next(&iter))) { + merge_ent = dir_rename_find_entry(dir_re_merge, head_ent->dir); + if (merge_ent && + !head_ent->non_unique_new_dir && + !merge_ent->non_unique_new_dir && + !strbuf_cmp(&head_ent->new_dir, &merge_ent->new_dir)) { + /* 1. Renamed identically; remove it from both sides */ + string_list_append(&remove_from_head, + head_ent->dir)->util = head_ent; + strbuf_release(&head_ent->new_dir); + string_list_append(&remove_from_merge, + merge_ent->dir)->util = merge_ent; + strbuf_release(&merge_ent->new_dir); + } else if (tree_has_path(head, head_ent->dir)) { + /* 2. This wasn't a directory rename after all */ + string_list_append(&remove_from_head, + head_ent->dir)->util = head_ent; + strbuf_release(&head_ent->new_dir); + } + } + + remove_hashmap_entries(dir_re_head, &remove_from_head); + remove_hashmap_entries(dir_re_merge, &remove_from_merge); + + hashmap_iter_init(dir_re_merge, &iter); + while ((merge_ent = hashma
[PATCH v10 34/36] merge-recursive: fix remainder of was_dirty() to use original index
was_dirty() uses was_tracked(), which has been updated to use the original index rather than the current one. However, was_dirty() also had a separate call to cache_file_exists(), causing it to still implicitly use the current index. Update that to instead use index_file_exists(). Also, was_dirty() had a hack where it would mark any file as non-dirty if we simply didn't know its modification time. This was due to using the current index rather than the original index, because D/F conflicts and such would cause unpack_trees() to not copy the modification times from the original index to the current one. Now that we are using the original index, we can dispense with this hack. Signed-off-by: Elijah Newren --- merge-recursive.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 097de7e5a7..1a481fa3dc 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -842,9 +842,9 @@ static int was_dirty(struct merge_options *o, const char *path) if (o->call_depth || !was_tracked(o, path)) return !dirty; - ce = cache_file_exists(path, strlen(path), ignore_case); - dirty = (ce->ce_stat_data.sd_mtime.sec > 0 && -verify_uptodate(ce, &o->unpack_opts) != 0); + ce = index_file_exists(o->unpack_opts.src_index, + path, strlen(path), ignore_case); + dirty = verify_uptodate(ce, &o->unpack_opts) != 0; return dirty; } -- 2.17.0.290.ge988e9ce2a
[PATCH v10 33/36] merge-recursive: fix was_tracked() to quit lying with some renamed paths
In commit aacb82de3ff8 ("merge-recursive: Split was_tracked() out of would_lose_untracked()", 2011-08-11), was_tracked() was split out of would_lose_untracked() with the intent to provide a function that could answer whether a path was tracked in the index before the merge. Sadly, it instead returned whether the path was in the working tree due to having been tracked in the index before the merge OR having been written there by unpack_trees(). The distinction is important when renames are involved, e.g. for a merge where: HEAD: modifies path b other: renames b->c In this case, c was not tracked in the index before the merge, but would have been added to the index at stage 0 and written to the working tree by unpack_trees(). would_lose_untracked() is more interested in the in-working-copy-for-either-reason behavior, while all other uses of was_tracked() want just was-it-tracked-in-index-before-merge behavior. Unsplit would_lose_untracked() and write a new was_tracked() function which answers whether a path was tracked in the index before the merge started. This will also affect was_dirty(), helping it to return better results since it can base answers off the original index rather than an index that possibly only copied over some of the stat information. However, was_dirty() will need an additional change that will be made in a subsequent patch. Signed-off-by: Elijah Newren --- This patch is nearly identical to one I sent out as an RFC and which was previously reviewed by Junio at https://public-inbox.org/git/CABPp-BFPTJsTUVoPxxN=2u5jeqn1ngddvmnhp+vlzktgzau...@mail.gmail.com/ It is not clear whether my responses in that thread were sufficient, but I did make the two changes I mentioned there: - Fix the broken comment in git_merge_trees() - Add a note to the comment in would_lose_untracked() about the annoying worktree-first-then-index requirement merge-recursive.c | 91 ++- merge-recursive.h | 1 + 2 files changed, 68 insertions(+), 24 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index b32e8d817a..097de7e5a7 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -344,6 +344,7 @@ static int git_merge_trees(struct merge_options *o, { int rc; struct tree_desc t[3]; + struct index_state tmp_index = { NULL }; memset(&o->unpack_opts, 0, sizeof(o->unpack_opts)); if (o->call_depth) @@ -354,7 +355,7 @@ static int git_merge_trees(struct merge_options *o, o->unpack_opts.head_idx = 2; o->unpack_opts.fn = threeway_merge; o->unpack_opts.src_index = &the_index; - o->unpack_opts.dst_index = &the_index; + o->unpack_opts.dst_index = &tmp_index; setup_unpack_trees_porcelain(&o->unpack_opts, "merge"); init_tree_desc_from_tree(t+0, common); @@ -362,13 +363,18 @@ static int git_merge_trees(struct merge_options *o, init_tree_desc_from_tree(t+2, merge); rc = unpack_trees(3, t, &o->unpack_opts); + cache_tree_free(&active_cache_tree); + /* -* unpack_trees NULLifies src_index, but it's used in verify_uptodate, -* so set to the new index which will usually have modification -* timestamp info copied over. +* Update the_index to match the new results, AFTER saving a copy +* in o->orig_index. Update src_index to point to the saved copy. +* (verify_uptodate() checks src_index, and the original index is +* the one that had the necessary modification timestamps.) */ - o->unpack_opts.src_index = &the_index; - cache_tree_free(&active_cache_tree); + o->orig_index = the_index; + the_index = tmp_index; + o->unpack_opts.src_index = &o->orig_index; + return rc; } @@ -773,31 +779,59 @@ static int dir_in_way(const char *path, int check_working_copy, int empty_ok) !(empty_ok && is_empty_dir(path)); } -static int was_tracked(const char *path) +/* + * Returns whether path was tracked in the index before the merge started + */ +static int was_tracked(struct merge_options *o, const char *path) { - int pos = cache_name_pos(path, strlen(path)); + int pos = index_name_pos(&o->orig_index, path, strlen(path)); if (0 <= pos) - /* we have been tracking this path */ + /* we were tracking this path before the merge */ return 1; - /* -* Look for an unmerged entry for the path, -* specifically stage #2, which would indicate -* that "our" side before the merge started -* had the path tracked (and resulted in a conflict). -*/ - for (pos = -1 - pos; -pos < active_nr && !strcmp(path, active_cache[pos]->name); -pos++) - if (ce_stage(active_cache[pos]) == 2) - return 1; return 0; } static int would_lose_untracked(const char *path) {
[PATCH v10 24/36] merge-recursive: avoid clobbering untracked files with directory renames
Reviewed-by: Stefan Beller Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-recursive.c | 42 +++-- t/t6043-merge-rename-directories.sh | 6 ++--- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index ec0bbcc3f4..c1c4faf61e 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1151,6 +1151,26 @@ static int conflict_rename_dir(struct merge_options *o, { const struct diff_filespec *dest = pair->two; + if (!o->call_depth && would_lose_untracked(dest->path)) { + char *alt_path = unique_path(o, dest->path, rename_branch); + + output(o, 1, _("Error: Refusing to lose untracked file at %s; " + "writing to %s instead."), + dest->path, alt_path); + /* +* Write the file in worktree at alt_path, but not in the +* index. Instead, write to dest->path for the index but +* only at the higher appropriate stage. +*/ + if (update_file(o, 0, &dest->oid, dest->mode, alt_path)) + return -1; + free(alt_path); + return update_stages(o, dest->path, NULL, +rename_branch == o->branch1 ? dest : NULL, +rename_branch == o->branch1 ? NULL : dest); + } + + /* Update dest->path both in index and in worktree */ if (update_file(o, 1, &dest->oid, dest->mode, dest->path)) return -1; return 0; @@ -1169,7 +1189,8 @@ static int handle_change_delete(struct merge_options *o, const char *update_path = path; int ret = 0; - if (dir_in_way(path, !o->call_depth, 0)) { + if (dir_in_way(path, !o->call_depth, 0) || + (!o->call_depth && would_lose_untracked(path))) { update_path = alt_path = unique_path(o, path, change_branch); } @@ -1295,6 +1316,12 @@ static int handle_file(struct merge_options *o, dst_name = unique_path(o, rename->path, cur_branch); output(o, 1, _("%s is a directory in %s adding as %s instead"), rename->path, other_branch, dst_name); + } else if (!o->call_depth && + would_lose_untracked(rename->path)) { + dst_name = unique_path(o, rename->path, cur_branch); + output(o, 1, _("Refusing to lose untracked file at %s; " + "adding as %s instead"), + rename->path, dst_name); } } if ((ret = update_file(o, 0, &rename->oid, rename->mode, dst_name))) @@ -1420,7 +1447,18 @@ static int conflict_rename_rename_2to1(struct merge_options *o, char *new_path2 = unique_path(o, path, ci->branch2); output(o, 1, _("Renaming %s to %s and %s to %s instead"), a->path, new_path1, b->path, new_path2); - remove_file(o, 0, path, 0); + if (would_lose_untracked(path)) + /* +* Only way we get here is if both renames were from +* a directory rename AND user had an untracked file +* at the location where both files end up after the +* two directory renames. See testcase 10d of t6043. +*/ + output(o, 1, _("Refusing to lose untracked file at " + "%s, even though it's in the way."), + path); + else + remove_file(o, 0, path, 0); ret = update_file(o, 0, &mfi_c1.oid, mfi_c1.mode, new_path1); if (!ret) ret = update_file(o, 0, &mfi_c2.oid, mfi_c2.mode, diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh index 3525c54bb4..0b60eb8053 100755 --- a/t/t6043-merge-rename-directories.sh +++ b/t/t6043-merge-rename-directories.sh @@ -2992,7 +2992,7 @@ test_expect_success '10b-setup: Overwrite untracked with dir rename + delete' ' ) ' -test_expect_failure '10b-check: Overwrite untracked with dir rename + delete' ' +test_expect_success '10b-check: Overwrite untracked with dir rename + delete' ' ( cd 10b && @@ -3070,7 +3070,7 @@ test_expect_success '10c-setup: Overwrite untracked with dir rename/rename(1to2) ) ' -test_expect_failure '10c-check: Overwrite untracked with dir rename/rename(1to2)' ' +test_expect_success '10c-check: Overwrite untracked with dir rename/rename(1to2)' ' ( cd 10c && @@ -3145,7 +3145,7 @@ test_expect_success '10d-setup: Delete untracked with dir rename/rename(2to1)
[PATCH v10 29/36] merge-recursive: improve add_cacheinfo error handling
Four closely related changes all with the purpose of fixing error handling in this function: - fix reported function name in add_cacheinfo error messages - differentiate between the two error messages - abort early when we hit the error (stop ignoring return code) - mark a test which was hitting this error as failing until we get the right fix In more detail... In commit 0424138d5715 ("Fix bogus error message from merge-recursive error path", 2007-04-01), it was noted that the name of the function which the error message claimed it was reported from did not match the actual function name. This was changed to something closer to the real function name, but it still didn't match the actual function name. Fix the reported name to match. Second, the two errors in this function had identical messages, preventing us from knowing which error had been triggered. Add a couple words to the second error message to differentiate the two. Next, make sure callers do not ignore the return code so that it will stop processing further entries (processing further entries could result in more output which could cause the error to scroll off the screen, or at least be missed by the user) and make it clear the error is the cause of the early abort. These errors should never be triggered in production; if either one is, it represents a bug in the calling path somewhere and is likely to have resulted in mis-merged content. The combination of ignoring of the return code and continuing to print other standard messages after hitting the error resulted in the following bug report from Junio: "...the command pretends that everything went well and merged cleanly in that path...[Behaving] in a buggy and unexplainable way is bad enough, doing so silently is unexcusable." Fix this. Finally, there was one test in the testsuite that did hit this error path, but was passing anyway. This would have been easy to miss since it had a test_must_fail and thus could have failed for the wrong reason, but in a separate testing step I added an intentional NULL-dereference to the codepath where these error messages are printed in order to flush out such cases. I could modify that test to explicitly check for this error and fail the test if it is hit, but since this test operates in a bit of a gray area and needed other changes, I went for a different fix. The gray area this test operates in is the following: If the merge of a certain file results in the same version of the file that existed in HEAD, but there are dirty modifications to the file, is that an error with a "Refusing to overwrite existing file" expected, or a case where the merge should succeed since we shouldn't have to touch the dirty file anyway? Recent discussion on the list leaned towards saying it should be a success. Therefore, change the expected behavior of this test to match. As a side effect, this makes the failed-due-to-hitting-add_cacheinfo-error very clear, and we can mark the test as test_expect_failure. A subsequent commit will implement the necessary changes to get this test to pass again. Signed-off-by: Elijah Newren --- I thought the changes were small enough to just combine, but after typing up the commit message and seeing how long it is, I'm wondering if I should split this into two commits. Thoughts? merge-recursive.c | 13 - t/t3501-revert-cherry-pick.sh | 7 +++ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 27278d51bb..b0f74cb243 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -316,7 +316,7 @@ static int add_cacheinfo(struct merge_options *o, ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage, 0); if (!ce) - return err(o, _("addinfo_cache failed for path '%s'"), path); + return err(o, _("add_cacheinfo failed for path '%s'; merge aborting."), path); ret = add_cache_entry(ce, options); if (refresh) { @@ -324,7 +324,7 @@ static int add_cacheinfo(struct merge_options *o, nce = refresh_cache_entry(ce, CE_MATCH_REFRESH | CE_MATCH_IGNORE_MISSING); if (!nce) - return err(o, _("addinfo_cache failed for path '%s'"), path); + return err(o, _("add_cacheinfo failed to refresh for path '%s'; merge aborting."), path); if (nce != ce) ret = add_cache_entry(nce, options); } @@ -942,7 +942,9 @@ static int update_file_flags(struct merge_options *o, } update_index: if (!ret && update_cache) - add_cacheinfo(o, mode, oid, path, 0, update_wd, ADD_CACHE_OK_TO_ADD); + if (add_cacheinfo(o, mode, oid, path, 0, update_wd, + ADD_CACHE_OK_TO_ADD)) + return -1; return ret; } @@ -2783,8 +2785,9 @@ static int merge_content(struct merge_options *o,
Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule
Hi Eddy, > I suspect that in the tests, because the "server side" repos are > local, the git fetch-by-sha1/cloning by hash will be done correctly, > without the need of a branch hint, but the problem will still exist > for servers such as github which do not support fetch-by-sha1. > In case I realize that a server-side repo that doesn't support > fetch-by-sha1 is needed, is there a mechanism to set that up in the > test case, or do I have to rethink my approach? You can force a clone (at least, not sure about fetch) to use the git protocol by --no-local, and then you can set uploadpack.{allowTipSHA1InWant, allowReachableSHA1InWant, allowAnySHA1InWant} as neded by the test. >From the fetch man page: For local repositories, also supported by Git natively, the following syntaxes may be used: · /path/to/repo.git/ · file:///path/to/repo.git/ These two syntaxes are mostly equivalent, except when cloning, when the former implies --local option. Thanks, Stefan
squash! Merge branch 'ps/test-chmtime-get'
Junio, you may want to squash this into your merge commit of branch ps/test-chmtime-get (today it is fa57c0871fc9) -- Hannes diff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c index daeddc1cbc..aa22af48c2 100644 --- a/t/helper/test-chmtime.c +++ b/t/helper/test-chmtime.c @@ -25,7 +25,7 @@ * * To print only the mtime use --get: * - * test-chmtime --get file + * test-tool chmtime --get file * * To set the mtime to current time: * @@ -33,7 +33,7 @@ * * To set the file mtime offset to +1 and print the new value: * - * test-chmtime --get +1 file + * test-tool chmtime --get +1 file * */ #include "test-tool.h"
[PATCH] doc/clone: update caption for GIT URLS cross-reference
The description of the argument directs readers to "See the URLS section below". When generating HTML this becomes a link to the "GIT URLS" section. When reading the man page in a terminal, the caption is slightly misleading. Use "GIT URLS" as the caption to avoid any confusion. Signed-off-by: Todd Zullinger --- Martin Ågren wrote: > On 18 April 2018 at 22:56, Todd Zullinger wrote: >> Subject: [PATCH] doc/clone: update caption for GIT URLS cross-reference >> >> The description of the argument directs readers to "See the >> URLS section below". When generating HTML this becomes a link to the >> "GIT URLS" section. When reading the man page in a terminal, the >> caption is slightly misleading. Use "GIT URLS" as the caption to avoid >> an confusion. > > s/an/any/? Indeed, thanks. >> The man page produced by asciidoc doesn't include hyperlinks. The >> description of the argument simply > > Abandoned first attempt at log message? ;-) That or it was when a squirrel ran by my window. ;) Thanks for catching both of these mistakes. Documentation/git-clone.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt index 42ca7b5095..b844b9957c 100644 --- a/Documentation/git-clone.txt +++ b/Documentation/git-clone.txt @@ -260,7 +260,7 @@ or `--mirror` is given) :: The (possibly remote) repository to clone from. See the - <> section below for more information on specifying + <> section below for more information on specifying repositories. :: -- 2.17.0 -- Todd ~~ Whenever you find yourself on the side of the majority, it is time to pause and reflect. -- Mark Twain
Re: [RFC PATCH 04/12] commit-graph: parse commit from chosen graph
Derrick Stolee writes: > Before checking a commit-graph file against the object database, we Actually there is quite a few checks more that can be done without accessing the object database... I'll take a look at later commits why this one is that relatively early in the series. > need to parse all commits from the given commit-graph file. Create > parse_commit_in_graph_one() to target a given struct commit_graph. > > Signed-off-by: Derrick Stolee > --- > commit-graph.c | 18 ++ > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/commit-graph.c b/commit-graph.c > index c5e5a0f860..6d0d303a7a 100644 > --- a/commit-graph.c > +++ b/commit-graph.c > @@ -308,17 +308,27 @@ static int find_commit_in_graph(struct commit *item, > struct commit_graph *g, uin > } > } > > -int parse_commit_in_graph(struct commit *item) > +int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item) > { > uint32_t pos; > > if (item->object.parsed) > - return 0; > + return 1; I am confused and befuddled by those apparent changes between returning 0 or returning 1 when object was parsed. > + > + if (find_commit_in_graph(item, g, &pos)) > + return fill_commit_in_graph(item, g, pos); > + > + return 0; > +} > + > +int parse_commit_in_graph(struct commit *item) > +{ > if (!core_commit_graph) > return 0; > + > prepare_commit_graph(); > - if (commit_graph && find_commit_in_graph(item, commit_graph, &pos)) > - return fill_commit_in_graph(item, commit_graph, pos); > + if (commit_graph) > + return parse_commit_in_graph_one(commit_graph, item); > return 0; > } Seems all right.
Re: [RFC PATCH 03/12] commit-graph: check file header information
Derrick Stolee writes: > During a run of 'git commit-graph check', list the issues with the > header information in the commit-graph file. Some of this information > is inferred from the loaded 'struct commit_graph'. > > Signed-off-by: Derrick Stolee > --- > commit-graph.c | 29 - > 1 file changed, 28 insertions(+), 1 deletion(-) > > diff --git a/commit-graph.c b/commit-graph.c > index cd0634bba0..c5e5a0f860 100644 > --- a/commit-graph.c > +++ b/commit-graph.c > @@ -820,7 +820,34 @@ void write_commit_graph(const char *obj_dir, > oids.nr = 0; > } > > +static int check_commit_graph_error; > +#define graph_report(...) { check_commit_graph_error = 1; > printf(__VA_ARGS__); } Shouldn't 'do { ... } while(0);' trick be used here, like e.g. for trace_performance macro? > + > int check_commit_graph(struct commit_graph *g) > { > - return !g; > + if (!g) { > + graph_report(_("no commit-graph file loaded")); > + return 1; > + } > + > + check_commit_graph_error = 0; > + The load_commit_graph_one() function does its own checks, some of whose are present below, and some of whose are missing. If it is used, then why duplicate tests - you would not get here as you would die earlier. If it is not used, then some tests are missing. > + if (get_be32(g->data) != GRAPH_SIGNATURE) > + graph_report(_("commit-graph file has incorrect header")); The load_commit_graph_one() shows more detailed information: "graph signature %X does not match signature %X", graph_signature, GRAPH_SIGNATURE) Also, load_commit_graph_one() checks that the file is not too short, and we actually can access whole header. > + > + if (*(g->data + 4) != 1) > + graph_report(_("commit-graph file version is not 1")); Again: "graph version %X does not match version %X", graph_version, GRAPH_VERSION Also, here we hardcode the commit-graph file version to 1. Accidentally, don't we offer backward compatibility, in that if git can read commit-graph file version 2, it can also read commit-graph file version 1? > + if (*(g->data + 5) != GRAPH_OID_VERSION) > + graph_report(_("commit-graph OID version is not 1 (SHA1)")); In one part we use symbolic constant, on the other hardcoded values. If GRAPH_OID_VERSION changes, what then? Also: "hash version %X does not match version %X", hash_version, GRAPH_OID_VERSION > + > + if (!g->chunk_oid_fanout) > + graph_report(_("commit-graph is missing the OID Fanout chunk")); > + if (!g->chunk_oid_lookup) > + graph_report(_("commit-graph is missing the OID Lookup chunk")); > + if (!g->chunk_commit_data) > + graph_report(_("commit-graph is missing the Commit Data > chunk")); All right. > + if (g->hash_len != GRAPH_OID_LEN) > + graph_report(_("commit-graph has incorrect hash length: %d"), > g->hash_len); We could be more detailed in error report: what hash length should be, then? > + > + return check_commit_graph_error; > } No tests of malformed commit-graph file?
pujcky
Jsem Ronald Bernstein, jsem úverový dustojník, dávám pujcky jednotlivcum a firme pro obchodní a osobní úcely, kontaktujte me, pokud potrebujete jakýkoliv druh pujcky. Poskytuji pujcky široké verejnosti s úrokovou sazbou 2%.
Re: [RFC PATCH 1/1] completion: Load completion file for external subcommand
On Mon, Apr 9, 2018 at 9:49 PM, Florian Gamböck wrote: > On 2018-04-09 11:26, Stefan Beller wrote: >> If Gits own completion script would be broken up by subcommand, would that >> also deliver an improvement in performance? > > > As it is now, the completion script is quite big. On a system with limited > resources, the initial loading time can be long and the memory footprint is > big, given that most users will just use a few commands. If you just use the > "commit" subcommand, the first loading of the two (smaller) scripts will be > slightly longer the first time (but not as long as with one big script, I > think), but the footprint will be drastically lower. The whole script is > 56kB big (without comments), after radically removing everything which is > not connected to _git_commit, it is only 11kB. > > So to answer your question: Yes. My first intuition is, that by splitting > the completion script and loading the sub-scripts dynamically, it will > improve in terms of speed and overall memory footprint, at least for the > average user that does not fire up all possible git commands. While "most users" might indeed only use a couple of git commands, they don't use systems so limited in resources where 56k vs. 11k makes a non-negligible difference. A system, where such a small difference is significant, must be so thight on resources that the user will most likely have issues running git anyway. I don't know how much memory Bash uses to store completion scripts, and it appears to be using a memory pool or something like our ALLOC_GROW, making measuring its memory footprint with simple means unusably inaccurate. Anyway, here are some numbers: # Baseline, bash does nothing: $ command time -f %M bash -c '' 2924 # Loading a <10k script: $ wc -c ./git-sh-setup.sh 9313 ./git-sh-setup.sh command time -f %M bash -c '. ./git-sh-setup.sh' 3724 # Loading our completion script; the increase in max memory # footprint is clearly not proportional to the size of the loaded # script: $ wc -c ./contrib/completion/git-completion.bash 69413 ./contrib/completion/git-completion.bash $ command time -f %M bash -c '. ./contrib/completion/git-completion.bash' 4092 # The main bash-completion script, though slightly smaller than ours, # appears to be requiring much more memory: $ wc -c /usr/share/bash-completion/bash_completion 67661 /usr/share/bash-completion/bash_completion $ command time -f %M bash -c './usr/share/bash-completion/bash_completion' 5952 # Loading both the main bash-completion script and our completion # script, which is where that memory pool/ALLOC_GROW-like thingy really # kicks in, as there is no real max memory footprint increase: $ command time -f %M bash -c '. /usr/share/bash-completion/bash_completion . ./contrib/completion/git-completion.bash' 5964 OTOH, our completion script has a couple of nice properties that we should keep working: - A user can simply run '. /path/to/git-completion.bash', and then completion for git commands will work. Even without bash-completion package, even in 'bash --norc'. If we were to split up our completion script, we would also have to roll our own __git_load_completion() funcion. - A user building Git from source doesn't have to install the completion script, it can be sourced from anywhere. And if the user chooses to install it somewhere, only a single file has to be copied. Currently we have completion functions for 55 git commands, which would mean 56 files to install if the completion script were split up. - Sourcing 'git-completion.bash' brings in all the latest and greatest. If it were split up, then sourcing it would only update the common functions, but not the completion functions of individual git commands. So we would have to take extra steps to delete those command-specific completion functions upon sourcing the completion script. However, we should be extra careful to delete only those completion functions that were source by the completion script, because users might have defined such functions in their '~/.bashrc'... I don't think it's worth it.
Bug Report - Pull remote branch does not retrieve new tags
What happens: When I create a new tag on the remote (changing nothing else) "git pull origin master" produces the following: From git.internal.company.com:team/testrepo * branchmaster -> FETCH_HEAD Already up-to-date. If I instead do a "git pull" I get: From git.internal.company.com:team/testrepo * [new tag] Testing11 -> Testing11 Already up-to-date. What I think should happen: The "git pull origin master" should retrieve the tag. This is with 2.16.2.windows.1, but also occurred on my previously installed version (2.12.2.windows.2) My understanding is that "git pull" and "git pull $repo $currentbranch" should function identically. Is this a bug, or am I misunderstanding the manual page? Thanks, Andy Confidentiality - This email is confidential. Not meant for you? - If you don't think this email is meant for you, please let us know. Do not copy or forward the information it contains, and delete this email from your system. Views expressed - Any personal views or opinions expressed in this email are the sender's, and do not necessarily reflect the views of Standard Life Aberdeen group. Monitoring - We filter and monitor emails to protect our systems and to keep them running smoothly. Emailing us - Email isn't a secure form of communication. If you want to send us confidential information please send it by post. However, if you do communicate with us by email on any subject, you are giving us permission to email you back. Phoning us - Calls may be monitored and/or recorded to protect both you and us and help with our training. Call charges will vary. Standard Life Aberdeen group - Standard Life Aberdeen group comprises Standard Life Aberdeen plc and its subsidiaries. For more information on Standard Life Aberdeen group visit our website http://www.standardlifeaberdeen.com/. Standard Life Aberdeen plc (SC286832), Standard Life Assurance Limited (SC286833) and Standard Life Employee Services Limited (SC271355) are all registered in Scotland at Standard Life House, 30 Lothian Road, Edinburgh EH1 2DH. Standard Life Assurance Limited is authorised by the Prudential Regulation Authority and regulated by the Financial Conduct Authority and the Prudential Regulation Authority. For more information on Standard Life Assurance limited visit our website http://www.standardlife.co.uk
Re: [RFC PATCH 02/12] commit-graph: add 'check' subcommand
Derrick Stolee writes: > If the commit-graph file becomes corrupt, we need a way to verify > its contents match the object database. In the manner of 'git fsck' > we will implement a 'git commit-graph check' subcommand to report > all issues with the file. Bikeshed: should the subcommand be called 'check' or 'verify'? > > Add the 'check' subcommand to the 'commit-graph' builtin and its > documentation. Add a simple test that ensures the command returns > a zero error code. It would be nice to have the information that the 'check' subcommand is currently a [almost no-op] stub in the subject... but that might not have been possible to fit. > > Signed-off-by: Derrick Stolee > --- > Documentation/git-commit-graph.txt | 7 +- > builtin/commit-graph.c | 38 ++ > commit-graph.c | 5 > commit-graph.h | 2 ++ > t/t5318-commit-graph.sh| 5 > 5 files changed, 56 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git-commit-graph.txt > b/Documentation/git-commit-graph.txt > index 4c97b555cc..93c7841ba2 100644 > --- a/Documentation/git-commit-graph.txt > +++ b/Documentation/git-commit-graph.txt > @@ -9,10 +9,10 @@ git-commit-graph - Write and verify Git commit graph files > SYNOPSIS > > [verse] > +'git commit-graph check' [--object-dir ] > 'git commit-graph read' [--object-dir ] > 'git commit-graph write' [--object-dir ] I still think that [--object-dir ] should be the optional parameter to the "git" wrapper, not to the "git commit-graph" command, i.e. 'git [--object-dir=] commit-graph ' But this can be done later, in a separate patch series. > > - Stray change. > DESCRIPTION > --- > > @@ -52,6 +52,11 @@ existing commit-graph file. > Read a graph file given by the commit-graph file and output basic > details about the graph file. Used for debugging purposes. > > +'check':: > + > +Read the commit-graph file and verify its contents against the object > +database. Used to check for corrupted data. > + I wonder if we should offer to verify file without checking against the object database (which is the costly part, I think). But this too can be added later if needed. > > EXAMPLES > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > index 37420ae0fd..77c1a04932 100644 > --- a/builtin/commit-graph.c > +++ b/builtin/commit-graph.c > @@ -7,11 +7,17 @@ > > static char const * const builtin_commit_graph_usage[] = { > N_("git commit-graph [--object-dir ]"), > + N_("git commit-graph check [--object-dir ]"), > N_("git commit-graph read [--object-dir ]"), > N_("git commit-graph write [--object-dir ] [--append] > [--stdin-packs|--stdin-commits]"), > NULL Isn't the case that each command would support the [--object-dir ] parameter? > }; > > +static const char * const builtin_commit_graph_check_usage[] = { > + N_("git commit-graph check [--object-dir ]"), > + NULL > +}; > + Looks good to me. > static const char * const builtin_commit_graph_read_usage[] = { > N_("git commit-graph read [--object-dir ]"), > NULL > @@ -29,6 +35,36 @@ static struct opts_commit_graph { > int append; > } opts; > > + > +static int graph_check(int argc, const char **argv) > +{ > + struct commit_graph *graph = 0; This is NULL, isn't it? Shouldn't it be stated as such? > + char *graph_name; > + > + static struct option builtin_commit_graph_check_options[] = { > + OPT_STRING(0, "object-dir", &opts.obj_dir, > +N_("dir"), > +N_("The object directory to store the graph")), This again is not the directory to _store_ the graph; this is the directory to _read_ graph from, or directory where the commit graph is _stored_. > + OPT_END(), > + }; > + > + argc = parse_options(argc, argv, NULL, > + builtin_commit_graph_check_options, > + builtin_commit_graph_check_usage, 0); > + > + if (!opts.obj_dir) > + opts.obj_dir = get_object_directory(); > + > + graph_name = get_commit_graph_filename(opts.obj_dir); > + graph = load_commit_graph_one(graph_name); > + > + if (!graph) > + die("graph file %s does not exist", graph_name); Shouldn't we quote pathname? Shouldn't this error message be marked for translation? Shouldn't we use "commit graph file" explicitly? > + FREE_AND_NULL(graph_name); > + > + return check_commit_graph(graph); > +} > + > static int graph_read(int argc, const char **argv) > { > struct commit_graph *graph = NULL; > @@ -160,6 +196,8 @@ int cmd_commit_graph(int argc, const char **argv, const > char *prefix) >PARSE_OPT_STOP_AT_NON_OPTION); > > if (argc > 0) { > + if (!strcmp(argv[0], "check")) > + return graph_check(argc, argv); >
[PATCH v7 03/17] sequencer: refactor how original todo list lines are accessed
Previously, we did a lot of arithmetic gymnastics to get at the line in the todo list (as stored in todo_list.buf). This might have been fast, but only in terms of execution speed, not in terms of developer time. Let's refactor this to make it a lot easier to read, and hence to reason about the correctness of the code. It is not performance-critical code anyway. Signed-off-by: Johannes Schindelin --- sequencer.c | 60 - 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/sequencer.c b/sequencer.c index 1ee70d843c1..3d0a45ab25a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1870,6 +1870,23 @@ static int count_commands(struct todo_list *todo_list) return count; } +static int get_item_line_offset(struct todo_list *todo_list, int index) +{ + return index < todo_list->nr ? + todo_list->items[index].offset_in_buf : todo_list->buf.len; +} + +static const char *get_item_line(struct todo_list *todo_list, int index) +{ + return todo_list->buf.buf + get_item_line_offset(todo_list, index); +} + +static int get_item_line_length(struct todo_list *todo_list, int index) +{ + return get_item_line_offset(todo_list, index + 1) + - get_item_line_offset(todo_list, index); +} + static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path) { int fd; @@ -2244,29 +2261,27 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) fd = hold_lock_file_for_update(&todo_lock, todo_path, 0); if (fd < 0) return error_errno(_("could not lock '%s'"), todo_path); - offset = next < todo_list->nr ? - todo_list->items[next].offset_in_buf : todo_list->buf.len; + offset = get_item_line_offset(todo_list, next); if (write_in_full(fd, todo_list->buf.buf + offset, todo_list->buf.len - offset) < 0) return error_errno(_("could not write to '%s'"), todo_path); if (commit_lock_file(&todo_lock) < 0) return error(_("failed to finalize '%s'"), todo_path); - if (is_rebase_i(opts)) { - const char *done_path = rebase_path_done(); - int fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666); - int prev_offset = !next ? 0 : - todo_list->items[next - 1].offset_in_buf; + if (is_rebase_i(opts) && next > 0) { + const char *done = rebase_path_done(); + int fd = open(done, O_CREAT | O_WRONLY | O_APPEND, 0666); + int ret = 0; - if (fd >= 0 && offset > prev_offset && - write_in_full(fd, todo_list->buf.buf + prev_offset, - offset - prev_offset) < 0) { - close(fd); - return error_errno(_("could not write to '%s'"), - done_path); - } - if (fd >= 0) - close(fd); + if (fd < 0) + return 0; + if (write_in_full(fd, get_item_line(todo_list, next - 1), + get_item_line_length(todo_list, next - 1)) + < 0) + ret = error_errno(_("could not write to '%s'"), done); + if (close(fd) < 0) + ret = error_errno(_("failed to finalize '%s'"), done); + return ret; } return 0; } @@ -3297,8 +3312,7 @@ int skip_unnecessary_picks(void) oid = &item->commit->object.oid; } if (i > 0) { - int offset = i < todo_list.nr ? - todo_list.items[i].offset_in_buf : todo_list.buf.len; + int offset = get_item_line_offset(&todo_list, i); const char *done_path = rebase_path_done(); fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666); @@ -3478,12 +3492,10 @@ int rearrange_squash(void) continue; while (cur >= 0) { - int offset = todo_list.items[cur].offset_in_buf; - int end_offset = cur + 1 < todo_list.nr ? - todo_list.items[cur + 1].offset_in_buf : - todo_list.buf.len; - char *bol = todo_list.buf.buf + offset; - char *eol = todo_list.buf.buf + end_offset; + const char *bol = + get_item_line(&todo_list, cur); + const char *eol = + get_item_line(&todo_list, cur + 1); /* replace 'pick', by 'fixup' or 'squash' */ command = todo_list.items[cur].command
[PATCH v7 14/17] rebase --rebase-merges: avoid "empty merges"
The `git merge` command does not allow merging commits that are already reachable from HEAD: `git merge HEAD^`, for example, will report that we are already up to date and not change a thing. In an interactive rebase, such a merge could occur previously, e.g. when competing (or slightly modified) versions of a patch series were applied upstream, and the user had to `git rebase --skip` all of the local commits, and the topic branch becomes "empty" as a consequence. Let's teach the todo command `merge` to behave the same as `git merge`. Seeing as it requires some low-level trickery to create such merges with Git's commands in the first place, we do not even have to bother to introduce an option to force `merge` to create such merge commits. Signed-off-by: Johannes Schindelin --- sequencer.c | 7 +++ t/t3430-rebase-merges.sh | 8 2 files changed, 15 insertions(+) diff --git a/sequencer.c b/sequencer.c index 620a4c3a506..708b8719965 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2802,6 +2802,13 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, write_message("no-ff", 5, git_path_merge_mode(), 0); bases = get_merge_bases(head_commit, merge_commit); + if (bases && !oidcmp(&merge_commit->object.oid, +&bases->item->object.oid)) { + ret = 0; + /* skip merging an ancestor of HEAD */ + goto leave_merge; + } + 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 dedfa09d761..37c3f73784a 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -192,4 +192,12 @@ test_expect_success 'post-rewrite hook and fixups work for merges' ' test_cmp expect actual ' +test_expect_success 'refuse to merge ancestors of HEAD' ' + echo "merge HEAD^" >script-from-scratch && + test_config -C wt sequence.editor \""$PWD"/replace-editor.sh\" && + before="$(git rev-parse HEAD)" && + git rebase -i HEAD && + test_cmp_rev HEAD $before +' + test_done -- 2.17.0.windows.1.4.g7e4058d72e3
[PATCH v7 04/17] sequencer: offer helpful advice when a command was rescheduled
Previously, we did that just magically, and potentially left some users quite puzzled. Let's err on the safe side instead, telling the user what is happening, and how they are supposed to continue. Signed-off-by: Johannes Schindelin --- sequencer.c | 16 1 file changed, 16 insertions(+) diff --git a/sequencer.c b/sequencer.c index 3d0a45ab25a..01443e0f245 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2555,6 +2555,17 @@ static const char *reflog_message(struct replay_opts *opts, return buf.buf; } +static const char rescheduled_advice[] = +N_("Could not execute the todo command\n" +"\n" +"%.*s" +"\n" +"It has been rescheduled; To edit the command before continuing, please\n" +"edit the todo list first:\n" +"\n" +"git rebase --edit-todo\n" +"git rebase --continue\n"); + static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) { int res = 0; @@ -2600,6 +2611,11 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) opts, is_final_fixup(todo_list)); if (is_rebase_i(opts) && res < 0) { /* Reschedule */ + advise(_(rescheduled_advice), + get_item_line_length(todo_list, + todo_list->current), + get_item_line(todo_list, +todo_list->current)); todo_list->current--; if (save_todo(todo_list, opts)) return -1; -- 2.17.0.windows.1.4.g7e4058d72e3
Re: [PATCH] git-send-email: Cc more people
- On Apr 19, 2018, at 8:10 AM, Matthew Wilcox wi...@infradead.org wrote: > On Thu, Apr 19, 2018 at 06:21:42AM +0900, Junio C Hamano wrote: >> Ævar Arnfjörð Bjarmason writes: >> >> > But IMO this patch is really lacking a few things before being ready: >> > >> > 1. You have no tests for this. See t/t9001-send-email.sh for examples, >> > ... >> > 2. Just a few lines down from your quoted hunk we have this: >> > ... code about $supress_cc{} ... >> >Your change should at least describe why those aren't being updated, >> >but probably we should add some other command-line option for >> >ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed >> >--[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by >> >a historical alias for --[no-]wildcard-by-cc=signed-off. >> > 3. Ditto all the documentation in "man git-send-email" about >> > ... >> >> Thanks, I agree that 2. (the lack of suppression) is a showstopper. > > I agree with that (and the lack of tests, obviously) > >> I'd further say that these new CC-sources should be disabled by >> default and made opt-in to avoid surprising existing users. > > But I disagree with this. The current behaviour is surprising to > existing users, to the point where people are writing their own scripts > to replace git send-email (which seems crazy to me). We could perhaps go with a whitelist approach. The four main match I would be tempted to add are: Acked-by, Reported-by, Reviewed-by, and Tested-by. My workflow is to initially CC a bunch of relevant maintainers when sending out a patch, and as the Acked, Reviewed and Tested by tags come it, I replace those CC with the relevant tag. I never expected them to stop being CC'd when switching between those categories. Thanks, Mathieu > >> One thing we also need to be very careful about is that some of the >> fields may not even have an e-mail address. We can expect that >> S-o-b and Cc would be of form "human readable name " >> by their nature, but it is perfectly fine to write only human >> readable name without address on random lines like "suggeted-by" and >> "helped-by". There needs a way for the end-user to avoid using data >> found on such lines as if they are valid e-mail addresses. > > I also agree with this. I'll add some test-cases and make sure we only > add these if they're valid email addresses. -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
[PATCH v7 15/17] pull: accept --rebase=merges to recreate the branch topology
Similar to the `preserve` mode simply passing the `--preserve-merges` option to the `rebase` command, the `merges` mode simply passes the `--rebase-merges` option. This will allow users to conveniently rebase non-trivial commit topologies when pulling new commits, without flattening them. Signed-off-by: Johannes Schindelin --- Documentation/config.txt | 8 Documentation/git-pull.txt | 5 - builtin/pull.c | 14 ++ builtin/remote.c | 18 ++ contrib/completion/git-completion.bash | 2 +- 5 files changed, 37 insertions(+), 10 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 2659153cb37..da46f154bb3 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1058,6 +1058,10 @@ branch..rebase:: "git pull" is run. See "pull.rebase" for doing this in a non branch-specific manner. + +When `merges`, pass the `--rebase-merges` option to 'git rebase' +so that locally committed merge commits will not be flattened +by running 'git pull'. ++ When preserve, also pass `--preserve-merges` along to 'git rebase' so that locally committed merge commits will not be flattened by running 'git pull'. @@ -2617,6 +2621,10 @@ pull.rebase:: pull" is run. See "branch..rebase" for setting this on a per-branch basis. + +When `merges`, pass the `--rebase-merges` option to 'git rebase' +so that locally committed merge commits will not be flattened +by running 'git pull'. ++ When preserve, also pass `--preserve-merges` along to 'git rebase' so that locally committed merge commits will not be flattened by running 'git pull'. diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt index ce05b7a5b13..6f76d815dd3 100644 --- a/Documentation/git-pull.txt +++ b/Documentation/git-pull.txt @@ -101,13 +101,16 @@ Options related to merging include::merge-options.txt[] -r:: ---rebase[=false|true|preserve|interactive]:: +--rebase[=false|true|merges|preserve|interactive]:: When true, rebase the current branch on top of the upstream branch after fetching. If there is a remote-tracking branch corresponding to the upstream branch and the upstream branch was rebased since last fetched, the rebase uses that information to avoid rebasing non-local changes. + +When set to `merges`, rebase using `git rebase --rebase-merges` so that +locally created merge commits will not be flattened. ++ When set to preserve, rebase with the `--preserve-merges` option passed to `git rebase` so that locally created merge commits will not be flattened. + diff --git a/builtin/pull.c b/builtin/pull.c index e32d6cd5b4c..70b44146ce4 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -27,14 +27,16 @@ enum rebase_type { REBASE_FALSE = 0, REBASE_TRUE, REBASE_PRESERVE, + REBASE_MERGES, REBASE_INTERACTIVE }; /** * Parses the value of --rebase. If value is a false value, returns * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is - * "preserve", returns REBASE_PRESERVE. If value is a invalid value, dies with - * a fatal error if fatal is true, otherwise returns REBASE_INVALID. + * "merges", returns REBASE_MERGES. If value is "preserve", returns + * REBASE_PRESERVE. If value is a invalid value, dies with a fatal error if + * fatal is true, otherwise returns REBASE_INVALID. */ static enum rebase_type parse_config_rebase(const char *key, const char *value, int fatal) @@ -47,6 +49,8 @@ static enum rebase_type parse_config_rebase(const char *key, const char *value, return REBASE_TRUE; else if (!strcmp(value, "preserve")) return REBASE_PRESERVE; + else if (!strcmp(value, "merges")) + return REBASE_MERGES; else if (!strcmp(value, "interactive")) return REBASE_INTERACTIVE; @@ -130,7 +134,7 @@ static struct option pull_options[] = { /* Options passed to git-merge or git-rebase */ OPT_GROUP(N_("Options related to merging")), { OPTION_CALLBACK, 'r', "rebase", &opt_rebase, - "false|true|preserve|interactive", + "false|true|merges|preserve|interactive", N_("incorporate changes by rebasing rather than merging"), PARSE_OPT_OPTARG, parse_opt_rebase }, OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL, @@ -800,7 +804,9 @@ static int run_rebase(const struct object_id *curr_head, argv_push_verbosity(&args); /* Options passed to git-rebase */ - if (opt_rebase == REBASE_PRESERVE) + if (opt_rebase == REBASE_MERGES) + argv_array_push(&args, "--rebase-merges"); + else if (opt_rebase == REBASE_PRESERVE) argv_array_push(&args, "--preserve-merges"); else if (opt_rebase == REBASE_INTERACTIVE) argv_array_push(&args, "--interactive")
[PATCH v7 17/17] rebase -i --rebase-merges: add a section to the man page
The --rebase-merges mode is probably not half as intuitive to use as its inventor hopes, so let's document it some. Signed-off-by: Johannes Schindelin --- Documentation/git-rebase.txt | 132 +++ 1 file changed, 132 insertions(+) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 8feadf6e663..0ff83b62821 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -389,6 +389,8 @@ By default, or when `no-rebase-cousins` was specified, commits which do not have `` as direct ancestor will keep their original branch point. If the `rebase-cousins` mode is turned on, such commits are rebased onto `` (or ``, if specified). ++ +See also REBASING MERGES below. -p:: --preserve-merges:: @@ -787,6 +789,136 @@ The ripple effect of a "hard case" recovery is especially bad: 'everyone' downstream from 'topic' will now have to perform a "hard case" recovery too! +REBASING MERGES +- + +The interactive rebase command was originally designed to handle +individual patch series. As such, it makes sense to exclude merge +commits from the todo list, as the developer may have merged the +current `master` while working on the branch, only to eventually +rebase all the commits onto `master` (skipping the merge commits). + +However, there are legitimate reasons why a developer may want to +recreate merge commits: to keep the branch structure (or "commit +topology") when working on multiple, inter-related branches. + +In the following example, the developer works on a topic branch that +refactors the way buttons are defined, and on another topic branch +that uses that refactoring to implement a "Report a bug" button. The +output of `git log --graph --format=%s -5` may look like this: + + +* Merge branch 'report-a-bug' +|\ +| * Add the feedback button +* | Merge branch 'refactor-button' +|\ \ +| |/ +| * Use the Button class for all buttons +| * Extract a generic Button class from the DownloadButton one + + +The developer might want to rebase those commits to a newer `master` +while keeping the branch topology, for example when the first topic +branch is expected to be integrated into `master` much earlier than the +second one, say, to resolve merge conflicts with changes to the +DownloadButton class that made it into `master`. + +This rebase can be performed using the `--rebase-merges` option. +It will generate a todo list looking like this: + + +label onto + +# Branch: refactor-button +reset onto +pick 123456 Extract a generic Button class from the DownloadButton one +pick 654321 Use the Button class for all buttons +label refactor-button + +# Branch: report-a-bug +reset refactor-button # Use the Button class for all buttons +pick abcdef Add the feedback button +label report-a-bug + +reset onto +merge -C a1b2c3 refactor-button # Merge 'refactor-button' +merge -C 6f5e4d report-a-bug # Merge 'report-a-bug' + + +In contrast to a regular interactive rebase, there are `label`, `reset` and +`merge` commands in addition to `pick` ones. + +The `label` command associates a label with the current HEAD when that +command is executed. These labels are created as worktree-local refs +(`refs/rewritten/`) that will be deleted when the rebase +finishes. That way, rebase operations in multiple worktrees linked to +the same repository do not interfere with one another. If the `label` command +fails, it is rescheduled immediately, with a helpful message how to proceed. + +The `reset` command is essentially a `git read-tree -m -u` (think: `git +reset --hard`, but refusing to overwrite untracked files) to the +specified revision (typically a previously-labeled one). If the `reset` +command fails, it is rescheduled immediately, with a helpful message how to +proceed. + +The `merge` command will merge the specified revision into whatever is +HEAD at that time. With `-C `, the commit message of +the specified merge commit will be used. When the `-C` is changed to +a lower-case `-c`, the message will be opened in an editor after a +successful merge so that the user can edit the message. + +If a `merge` command fails for any reason other than merge conflicts (i.e. +when the merge operation did not even start), it is rescheduled immediately. + +At this time, the `merge` command will *always* use the `recursive` +merge strategy, with no way to choose a different one. To work around +this, an `exec` command can be used to call `git merge` explicitly, +using the fact that the labels are worktree-local refs (the ref +`refs/rewritten/onto` would correspond to the label `onto`). + +Note: the first command (`label onto`) labels the revision onto which +the commits are rebased; The name `onto` is just a convention, as a nod +to the `--onto` option. + +It is also possible to introduce completely new merge commits from scratch +by adding a command of the form `merge `. This form will +generate a tentative commit
[PATCH v7 16/17] rebase -i: introduce --rebase-merges=[no-]rebase-cousins
This one is a bit tricky to explain, so let's try with a diagram: C / \ A - B - E - F \ / D To illustrate what this new mode is all about, let's consider what happens upon `git rebase -i --rebase-merges B`, in particular to the commit `D`. So far, the new branch structure would be: --- C' -- / \ A - B -- E' - F' \/ D' This is not really preserving the branch topology from before! The reason is that the commit `D` does not have `B` as ancestor, and therefore it gets rebased onto `B`. This is unintuitive behavior. Even worse, when recreating branch structure, most use cases would appear to want cousins *not* to be rebased onto the new base commit. For example, Git for Windows (the heaviest user of the Git garden shears, which served as the blueprint for --rebase-merges) frequently merges branches from `next` early, and these branches certainly do *not* want to be rebased. In the example above, the desired outcome would look like this: --- C' -- / \ A - B -- E' - F' \/ -- D' -- Let's introduce the term "cousins" for such commits ("D" in the example), and let's not rebase them by default, introducing the new "rebase-cousins" mode for use cases where they should be rebased. Signed-off-by: Johannes Schindelin --- Documentation/git-rebase.txt | 7 ++- builtin/rebase--helper.c | 9 - git-rebase--interactive.sh | 1 + git-rebase.sh| 12 +++- sequencer.c | 4 sequencer.h | 6 ++ t/t3430-rebase-merges.sh | 18 ++ 7 files changed, 54 insertions(+), 3 deletions(-) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 936c5619b42..8feadf6e663 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -379,11 +379,16 @@ rebase.instructionFormat. A customized instruction format will automatically have the long commit hash prepended to the format. -r:: ---rebase-merges:: +--rebase-merges[=(rebase-cousins|no-rebase-cousins)]:: Rebase merge commits instead of flattening the history by replaying merges. Merge conflict resolutions or manual amendments to merge commits are not rebased automatically, but have to be applied manually. ++ +By default, or when `no-rebase-cousins` was specified, commits which do not +have `` as direct ancestor will keep their original branch point. +If the `rebase-cousins` mode is turned on, such commits are rebased onto +`` (or ``, if specified). -p:: --preserve-merges:: diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c index 781782e7272..f7c2a5fdc81 100644 --- a/builtin/rebase--helper.c +++ b/builtin/rebase--helper.c @@ -13,7 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix) { struct replay_opts opts = REPLAY_OPTS_INIT; unsigned flags = 0, keep_empty = 0, rebase_merges = 0; - int abbreviate_commands = 0; + int abbreviate_commands = 0, rebase_cousins = -1; enum { CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS, CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH, @@ -25,6 +25,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message, N_("allow commits with empty messages")), OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")), + OPT_BOOL(0, "rebase-cousins", &rebase_cousins, +N_("keep original branch points of cousins")), OPT_CMDMODE(0, "continue", &command, N_("continue rebase"), CONTINUE), OPT_CMDMODE(0, "abort", &command, N_("abort rebase"), @@ -59,8 +61,13 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix) flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0; flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0; flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0; + flags |= rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0; flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0; + if (rebase_cousins >= 0 && !rebase_merges) + warning(_("--[no-]rebase-cousins has no effect without " + "--rebase-merges")); + if (command == CONTINUE && argc == 1) return !!sequencer_continue(&opts); if (command == ABORT && argc == 1) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 7a3daf3e40c..b4ad130e8b1 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -971,6 +971,7 @@ git_rebase__interactive () { git rebase--helper --make-script ${keep_empty:+--keep-empty} \ ${rebase_merges:+--rebase-merges} \
[PATCH v7 13/17] sequencer: handle post-rewrite for merge commands
In the previous patches, we implemented the basic functionality of the `git rebase -i --rebase-merges` command, in particular the `merge` command to create merge commits in the sequencer. The interactive rebase is a lot more these days, though, than a simple cherry-pick in a loop. For example, it calls the post-rewrite hook (if any) after rebasing with a mapping of the old->new commits. This patch implements the post-rewrite handling for the `merge` command we just introduced. The other commands that were added recently (`label` and `reset`) do not create new commits, therefore post-rewrite hooks do not need to handle them. Signed-off-by: Johannes Schindelin --- sequencer.c | 3 +++ t/t3430-rebase-merges.sh | 25 + 2 files changed, 28 insertions(+) diff --git a/sequencer.c b/sequencer.c index 32ebbc002c1..620a4c3a506 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3061,6 +3061,9 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) item->flags, opts); if (res < 0) goto reschedule; + if (item->commit) + record_in_rewritten(&item->commit->object.oid, + peek_command(todo_list, 1)); } else if (!is_noop(item->command)) return error(_("unknown command %d"), item->command); diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 4b553fc78b1..dedfa09d761 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -167,4 +167,29 @@ test_expect_success 'refs/rewritten/* is worktree-local' ' test_cmp_rev HEAD "$(cat wt/b)" ' +test_expect_success 'post-rewrite hook and fixups work for merges' ' + git checkout -b post-rewrite && + test_commit same1 && + git reset --hard HEAD^ && + test_commit same2 && + git merge -m "to fix up" same1 && + echo same old same old >same2.t && + test_tick && + git commit --fixup HEAD same2.t && + fixup="$(git rev-parse HEAD)" && + + mkdir -p .git/hooks && + test_when_finished "rm .git/hooks/post-rewrite" && + echo "cat >actual" | write_script .git/hooks/post-rewrite && + + test_tick && + git rebase -i --autosquash -r HEAD^^^ && + printf "%s %s\n%s %s\n%s %s\n%s %s\n" >expect $(git rev-parse \ + $fixup^^2 HEAD^2 \ + $fixup^^ HEAD^ \ + $fixup^ HEAD \ + $fixup HEAD) && + test_cmp expect actual +' + test_done -- 2.17.0.windows.1.4.g7e4058d72e3
[PATCH v7 11/17] rebase --rebase-merges: add test for --keep-empty
From: Phillip Wood If there are empty commits on the left hand side of $upstream...HEAD then the empty commits on the right hand side that we want to keep are being pruned. Signed-off-by: Phillip Wood --- t/t3421-rebase-topology-linear.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh index 68fe2003ef5..fbae5dab7e2 100755 --- a/t/t3421-rebase-topology-linear.sh +++ b/t/t3421-rebase-topology-linear.sh @@ -217,6 +217,7 @@ test_run_rebase success '' test_run_rebase failure -m test_run_rebase failure -i test_run_rebase failure -p +test_run_rebase success --rebase-merges # m # / -- 2.17.0.windows.1.4.g7e4058d72e3
[PATCH v7 12/17] sequencer: make refs generated by the `label` command worktree-local
This allows for rebases to be run in parallel in separate worktrees (think: interrupted in the middle of one rebase, being asked to perform a different rebase, adding a separate worktree just for that job). Signed-off-by: Johannes Schindelin --- refs.c | 3 ++- t/t3430-rebase-merges.sh | 14 ++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 8b7a77fe5ee..f61ec58d1df 100644 --- a/refs.c +++ b/refs.c @@ -600,7 +600,8 @@ int dwim_log(const char *str, int len, struct object_id *oid, char **log) static int is_per_worktree_ref(const char *refname) { return !strcmp(refname, "HEAD") || - starts_with(refname, "refs/bisect/"); + starts_with(refname, "refs/bisect/") || + starts_with(refname, "refs/rewritten/"); } static int is_pseudoref_syntax(const char *refname) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index e80fa068d05..4b553fc78b1 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -153,4 +153,18 @@ test_expect_success 'with a branch tip that was cherry-picked already' ' EOF ' +test_expect_success 'refs/rewritten/* is worktree-local' ' + git worktree add wt && + cat >wt/script-from-scratch <<-\EOF && + label xyz + exec GIT_DIR=../.git git rev-parse --verify refs/rewritten/xyz >a || : + exec git rev-parse --verify refs/rewritten/xyz >b + EOF + + test_config -C wt sequence.editor \""$PWD"/replace-editor.sh\" && + git -C wt rebase -i HEAD && + test_must_be_empty wt/a && + test_cmp_rev HEAD "$(cat wt/b)" +' + test_done -- 2.17.0.windows.1.4.g7e4058d72e3
[PATCH v7 02/17] sequencer: make rearrange_squash() a bit more obvious
There are some commands that have to be skipped from rearranging by virtue of not handling any commits. However, the logic was not quite obvious: it skipped commands based on their position in the enum todo_command. Instead, let's make it explicit that we skip all commands that do not handle any commit. With one exception: the `drop` command, because it, well, drops the commit and is therefore not eligible to rearranging. Note: this is a bit academic at the moment because the only time we call `rearrange_squash()` is directly after generating the todo list, when we have nothing but `pick` commands anyway. However, the upcoming `merge` command *will* want to be handled by that function, and it *can* handle commits. Signed-off-by: Johannes Schindelin --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 096e6d241e0..1ee70d843c1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3393,7 +3393,7 @@ int rearrange_squash(void) struct subject2item_entry *entry; next[i] = tail[i] = -1; - if (item->command >= TODO_EXEC) { + if (!item->commit || item->command == TODO_DROP) { subjects[i] = NULL; continue; } -- 2.17.0.windows.1.4.g7e4058d72e3
[PATCH v7 10/17] rebase: introduce the --rebase-merges option
Once upon a time, this here developer thought: wouldn't it be nice if, say, Git for Windows' patches on top of core Git could be represented as a thicket of branches, and be rebased on top of core Git in order to maintain a cherry-pick'able set of patch series? The original attempt to answer this was: git rebase --preserve-merges. However, that experiment was never intended as an interactive option, and it only piggy-backed on git rebase --interactive because that command's implementation looked already very, very familiar: it was designed by the same person who designed --preserve-merges: yours truly. Some time later, some other developer (I am looking at you, Andreas! ;-)) decided that it would be a good idea to allow --preserve-merges to be combined with --interactive (with caveats!) and the Git maintainer (well, the interim Git maintainer during Junio's absence, that is) agreed, and that is when the glamor of the --preserve-merges design started to fall apart rather quickly and unglamorously. The reason? In --preserve-merges mode, the parents of a merge commit (or for that matter, of *any* commit) were not stated explicitly, but were *implied* by the commit name passed to the `pick` command. This made it impossible, for example, to reorder commits. Not to mention to flatten the branch topology or, deity forbid, to split topic branches into two. Alas, these shortcomings also prevented that mode (whose original purpose was to serve Git for Windows' needs, with the additional hope that it may be useful to others, too) from serving Git for Windows' needs. Five years later, when it became really untenable to have one unwieldy, big hodge-podge patch series of partly related, partly unrelated patches in Git for Windows that was rebased onto core Git's tags from time to time (earning the undeserved wrath of the developer of the ill-fated git-remote-hg series that first obsoleted Git for Windows' competing approach, only to be abandoned without maintainer later) was really untenable, the "Git garden shears" were born [*1*/*2*]: a script, piggy-backing on top of the interactive rebase, that would first determine the branch topology of the patches to be rebased, create a pseudo todo list for further editing, transform the result into a real todo list (making heavy use of the `exec` command to "implement" the missing todo list commands) and finally recreate the patch series on top of the new base commit. That was in 2013. And it took about three weeks to come up with the design and implement it as an out-of-tree script. Needless to say, the implementation needed quite a few years to stabilize, all the while the design itself proved itself sound. With this patch, the goodness of the Git garden shears comes to `git rebase -i` itself. Passing the `--rebase-merges` option will generate a todo list that can be understood readily, and where it is obvious how to reorder commits. New branches can be introduced by inserting `label` commands and calling `merge `. And once this mode will have become stable and universally accepted, we can deprecate the design mistake that was `--preserve-merges`. Link *1*: https://github.com/msysgit/msysgit/blob/master/share/msysGit/shears.sh Link *2*: https://github.com/git-for-windows/build-extra/blob/master/shears.sh Signed-off-by: Johannes Schindelin --- Documentation/git-rebase.txt | 10 +- contrib/completion/git-completion.bash | 2 +- git-rebase--interactive.sh | 1 + git-rebase.sh | 6 + t/t3430-rebase-merges.sh | 156 + 5 files changed, 173 insertions(+), 2 deletions(-) create mode 100755 t/t3430-rebase-merges.sh diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 3277ca14327..936c5619b42 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -378,6 +378,13 @@ The commit list format can be changed by setting the configuration option rebase.instructionFormat. A customized instruction format will automatically have the long commit hash prepended to the format. +-r:: +--rebase-merges:: + Rebase merge commits instead of flattening the history by replaying + merges. Merge conflict resolutions or manual amendments to merge + commits are not rebased automatically, but have to be applied + manually. + -p:: --preserve-merges:: Recreate merge commits instead of flattening the history by replaying @@ -780,7 +787,8 @@ BUGS The todo list presented by `--preserve-merges --interactive` does not represent the topology of the revision graph. Editing commits and rewording their commit messages should work fine, but attempts to -reorder commits tend to produce counterintuitive results. +reorder commits tend to produce counterintuitive results. Use +--rebase-merges for a more faithful representation. For example, an attempt to rearrange diff --git a/contrib/completion/git-completion.bas
[PATCH v7 09/17] rebase-helper --make-script: introduce a flag to rebase merges
The sequencer just learned new commands intended to recreate branch structure (similar in spirit to --preserve-merges, but with a substantially less-broken design). Let's allow the rebase--helper to generate todo lists making use of these commands, triggered by the new --rebase-merges option. For a commit topology like this (where the HEAD points to C): - A - B - C \ / D the generated todo list would look like this: # branch D pick 0123 A label branch-point pick 1234 D label D reset branch-point pick 2345 B merge -C 3456 D # C To keep things simple, we first only implement support for merge commits with exactly two parents, leaving support for octopus merges to a later patch series. As a special, hard-coded label, all merge-rebasing todo lists start with the command `label onto` so that we can later always refer to the revision onto which everything is rebased. Signed-off-by: Johannes Schindelin --- builtin/rebase--helper.c | 4 +- sequencer.c | 351 ++- sequencer.h | 1 + 3 files changed, 353 insertions(+), 3 deletions(-) diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c index ad074705bb5..781782e7272 100644 --- a/builtin/rebase--helper.c +++ b/builtin/rebase--helper.c @@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] = { int cmd_rebase__helper(int argc, const char **argv, const char *prefix) { struct replay_opts opts = REPLAY_OPTS_INIT; - unsigned flags = 0, keep_empty = 0; + unsigned flags = 0, keep_empty = 0, rebase_merges = 0; int abbreviate_commands = 0; enum { CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS, @@ -24,6 +24,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")), OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message, N_("allow commits with empty messages")), + OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")), OPT_CMDMODE(0, "continue", &command, N_("continue rebase"), CONTINUE), OPT_CMDMODE(0, "abort", &command, N_("abort rebase"), @@ -57,6 +58,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix) flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0; flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0; + flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0; flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0; if (command == CONTINUE && argc == 1) diff --git a/sequencer.c b/sequencer.c index 2ae2294272b..32ebbc002c1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -25,6 +25,8 @@ #include "sigchain.h" #include "unpack-trees.h" #include "worktree.h" +#include "oidmap.h" +#include "oidset.h" #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" @@ -3414,6 +3416,343 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag) strbuf_release(&sob); } +struct labels_entry { + struct hashmap_entry entry; + char label[FLEX_ARRAY]; +}; + +static int labels_cmp(const void *fndata, const struct labels_entry *a, + const struct labels_entry *b, const void *key) +{ + return key ? strcmp(a->label, key) : strcmp(a->label, b->label); +} + +struct string_entry { + struct oidmap_entry entry; + char string[FLEX_ARRAY]; +}; + +struct label_state { + struct oidmap commit2label; + struct hashmap labels; + struct strbuf buf; +}; + +static const char *label_oid(struct object_id *oid, const char *label, +struct label_state *state) +{ + struct labels_entry *labels_entry; + struct string_entry *string_entry; + struct object_id dummy; + size_t len; + int i; + + string_entry = oidmap_get(&state->commit2label, oid); + if (string_entry) + return string_entry->string; + + /* +* For "uninteresting" commits, i.e. commits that are not to be +* rebased, and which can therefore not be labeled, we use a unique +* abbreviation of the commit name. This is slightly more complicated +* than calling find_unique_abbrev() because we also need to make +* sure that the abbreviation does not conflict with any other +* label. +* +* We disallow "interesting" commits to be labeled by a string that +* is a valid full-length hash, to ensure that we always can find an +* abbreviation for any uninteresting commit's names that does not +* clash with any other label. +*/ + if (!label) { + char *p; + + strbuf_reset(&state->buf); +
[PATCH v7 08/17] sequencer: fast-forward `merge` commands, if possible
Just like with regular `pick` commands, if we are trying to rebase a merge commit, we now test whether the parents of said commit match HEAD and the commits to be merged, and fast-forward if possible. This is not only faster, but also avoids unnecessary proliferation of new objects. Signed-off-by: Johannes Schindelin --- sequencer.c | 33 - 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 90b2fac96b1..2ae2294272b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2679,7 +2679,7 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, struct commit *head_commit, *merge_commit, *i; struct commit_list *bases, *j, *reversed = NULL; struct merge_options o; - int merge_arg_len, oneline_offset, ret; + int merge_arg_len, oneline_offset, can_fast_forward, ret; static struct lock_file lock; const char *p; @@ -2764,6 +2764,37 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, } } + /* +* If HEAD is not identical to the first parent of the original merge +* commit, we cannot fast-forward. +*/ + can_fast_forward = opts->allow_ff && commit && commit->parents && + !oidcmp(&commit->parents->item->object.oid, + &head_commit->object.oid); + + /* +* If the merge head is different from the original one, we cannot +* fast-forward. +*/ + if (can_fast_forward) { + struct commit_list *second_parent = commit->parents->next; + + if (second_parent && !second_parent->next && + oidcmp(&merge_commit->object.oid, + &second_parent->item->object.oid)) + can_fast_forward = 0; + } + + if (can_fast_forward && commit->parents->next && + !commit->parents->next->next && + !oidcmp(&commit->parents->next->item->object.oid, + &merge_commit->object.oid)) { + rollback_lock_file(&lock); + ret = fast_forward_to(&commit->object.oid, + &head_commit->object.oid, 0, opts); + goto leave_merge; + } + write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ, git_path_merge_head(), 0); write_message("no-ff", 5, git_path_merge_mode(), 0); -- 2.17.0.windows.1.4.g7e4058d72e3
[PATCH v7 07/17] # This is a combination of 2 commits. # This is the 1st commit message:
sequencer: introduce the `merge` command This patch is part of the effort to reimplement `--preserve-merges` with a substantially improved design, a design that has been developed in the Git for Windows project to maintain the dozens of Windows-specific patch series on top of upstream Git. The previous patch implemented the `label` and `reset` commands to label commits and to reset to labeled commits. This patch adds the `merge` command, with the following syntax: merge [-C ] # The parameter in this instance is the *original* merge commit, whose author and message will be used for the merge commit that is about to be created. The parameter refers to the (possibly rewritten) revision to merge. Let's see an example of a todo list: label onto # Branch abc reset onto pick deadbeef Hello, world! label abc reset onto pick cafecafe And now for something completely different merge -C baaabaaa abc # Merge the branch 'abc' into master To edit the merge commit's message (a "reword" for merges, if you will), use `-c` (lower-case) instead of `-C`; this convention was borrowed from `git commit` that also supports `-c` and `-C` with similar meanings. To create *new* merges, i.e. without copying the commit message from an existing commit, simply omit the `-C ` parameter (which will open an editor for the merge message): merge abc This comes in handy when splitting a branch into two or more branches. Note: this patch only adds support for recursive merges, to keep things simple. Support for octopus merges will be added later in a separate patch series, support for merges using strategies other than the recursive merge is left for the future. Signed-off-by: Johannes Schindelin # The commit message #2 will be skipped: # fixup! sequencer: introduce the `merge` command --- git-rebase--interactive.sh | 4 + sequencer.c| 184 + 2 files changed, 188 insertions(+) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index e8d3a7d7588..ccd5254d1c9 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -164,6 +164,10 @@ x, exec = run command (the rest of the line) using shell d, drop = remove commit l, label = label current HEAD with a name t, reset = reset HEAD to a label +m, merge [-C | -c ] [# ] +. create a merge commit using the original merge commit's +. message (or the oneline, if no original merge commit was +. specified). Use -c to reword the commit message. These lines can be re-ordered; they are executed from top to bottom. " | git stripspace --comment-lines >>"$todo" diff --git a/sequencer.c b/sequencer.c index 9e09026b594..90b2fac96b1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1305,6 +1305,7 @@ enum todo_command { TODO_EXEC, TODO_LABEL, TODO_RESET, + TODO_MERGE, /* commands that do nothing but are counted for reporting progress */ TODO_NOOP, TODO_DROP, @@ -1325,6 +1326,7 @@ static struct { { 'x', "exec" }, { 'l', "label" }, { 't', "reset" }, + { 'm', "merge" }, { 0, "noop" }, { 'd', "drop" }, { 0, NULL } @@ -1752,9 +1754,14 @@ static int read_and_refresh_cache(struct replay_opts *opts) return 0; } +enum todo_item_flags { + TODO_EDIT_MERGE_MSG = 1 +}; + struct todo_item { enum todo_command command; struct commit *commit; + unsigned int flags; const char *arg; int arg_len; size_t offset_in_buf; @@ -1789,6 +1796,8 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol) char *end_of_object_name; int i, saved, status, padding; + item->flags = 0; + /* left-trim */ bol += strspn(bol, " \t"); @@ -1838,6 +1847,21 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol) return 0; } + if (item->command == TODO_MERGE) { + if (skip_prefix(bol, "-C", &bol)) + bol += strspn(bol, " \t"); + else if (skip_prefix(bol, "-c", &bol)) { + bol += strspn(bol, " \t"); + item->flags |= TODO_EDIT_MERGE_MSG; + } else { + item->flags |= TODO_EDIT_MERGE_MSG; + item->commit = NULL; + item->arg = bol; + item->arg_len = (int)(eol - bol); + return 0; + } + } + end_of_object_name = (char *) bol + strcspn(bol, " \t\n"); saved = *end_of_object_name; *end_of_object_name = '\0'; @@ -2646,6 +2670,153 @@ static int do_reset(const char *name, int len, struct replay_opts *opts) return ret; } +static int do_merge(struct commit *commit, const char *arg, int arg_len, +
[PATCH v7 06/17] sequencer: introduce new commands to reset the revision
In the upcoming commits, we will teach the sequencer to rebase merges. This will be done in a very different way from the unfortunate design of `git rebase --preserve-merges` (which does not allow for reordering commits, or changing the branch topology). The main idea is to introduce new todo list commands, to support labeling the current revision with a given name, resetting the current revision to a previous state, and merging labeled revisions. This idea was developed in Git for Windows' Git garden shears (that are used to maintain Git for Windows' "thicket of branches" on top of upstream Git), and this patch is part of the effort to make it available to a wider audience, as well as to make the entire process more robust (by implementing it in a safe and portable language rather than a Unix shell script). This commit implements the commands to label, and to reset to, given revisions. The syntax is: label reset Internally, the `label ` command creates the ref `refs/rewritten/`. This makes it possible to work with the labeled revisions interactively, or in a scripted fashion (e.g. via the todo list command `exec`). These temporary refs are removed upon sequencer_remove_state(), so that even a `git rebase --abort` cleans them up. We disallow '#' as label because that character will be used as separator in the upcoming `merge` command. Later in this patch series, we will mark the `refs/rewritten/` refs as worktree-local, to allow for interactive rebases to be run in parallel in worktrees linked to the same repository. As typos happen, a failed `label` or `reset` command will be rescheduled immediately. Note that this needs a little change in the original code to perform a reschedule: there is no commit from which to generate a patch here (and we will simply fall through to the regular `return res`). We keep that code path, though, because we will use it for the upcoming `merge` command, too. Signed-off-by: Johannes Schindelin --- git-rebase--interactive.sh | 2 + sequencer.c| 201 +++-- 2 files changed, 196 insertions(+), 7 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index e1b865f43f2..e8d3a7d7588 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -162,6 +162,8 @@ s, squash = use commit, but meld into previous commit f, fixup = like \"squash\", but discard this commit's log message x, exec = run command (the rest of the line) using shell d, drop = remove commit +l, label = label current HEAD with a name +t, reset = reset HEAD to a label These lines can be re-ordered; they are executed from top to bottom. " | git stripspace --comment-lines >>"$todo" diff --git a/sequencer.c b/sequencer.c index 01443e0f245..9e09026b594 100644 --- a/sequencer.c +++ b/sequencer.c @@ -23,6 +23,8 @@ #include "hashmap.h" #include "notes-utils.h" #include "sigchain.h" +#include "unpack-trees.h" +#include "worktree.h" #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" @@ -120,6 +122,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha") static GIT_PATH_FUNC(rebase_path_rewritten_list, "rebase-merge/rewritten-list") static GIT_PATH_FUNC(rebase_path_rewritten_pending, "rebase-merge/rewritten-pending") + +/* + * The path of the file listing refs that need to be deleted after the rebase + * finishes. This is used by the `label` command to record the need for cleanup. + */ +static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete") + /* * The following files are written by git-rebase just after parsing the * command-line (and are only consumed, not modified, by the sequencer). @@ -244,18 +253,33 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts) int sequencer_remove_state(struct replay_opts *opts) { - struct strbuf dir = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; int i; + if (strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) { + char *p = buf.buf; + while (*p) { + char *eol = strchr(p, '\n'); + if (eol) + *eol = '\0'; + if (delete_ref("(rebase -i) cleanup", p, NULL, 0) < 0) + warning(_("could not delete '%s'"), p); + if (!eol) + break; + p = eol + 1; + } + } + free(opts->gpg_sign); free(opts->strategy); for (i = 0; i < opts->xopts_nr; i++) free(opts->xopts[i]); free(opts->xopts); - strbuf_addstr(&dir, get_dir(opts)); - remove_dir_recursively(&dir, 0); - strbuf_release(&dir); + strbuf_reset(&buf); + strbuf_addstr(&buf, get_dir(opts)); + remove_dir_recursively(&buf, 0); + strbuf_release(&buf); return 0; } @@ -
[PATCH v7 05/17] git-rebase--interactive: clarify arguments
From: Stefan Beller Up to now each command took a commit as its first argument and ignored the rest of the line (usually the subject of the commit) Now that we are about to introduce commands that take different arguments, clarify each command by giving the argument list. Signed-off-by: Stefan Beller Signed-off-by: Johannes Schindelin --- git-rebase--interactive.sh | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 50323fc2735..e1b865f43f2 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -155,13 +155,13 @@ reschedule_last_action () { append_todo_help () { gettext " Commands: -p, pick = use commit -r, reword = use commit, but edit the commit message -e, edit = use commit, but stop for amending -s, squash = use commit, but meld into previous commit -f, fixup = like \"squash\", but discard this commit's log message -x, exec = run command (the rest of the line) using shell -d, drop = remove commit +p, pick = use commit +r, reword = use commit, but edit the commit message +e, edit = use commit, but stop for amending +s, squash = use commit, but meld into previous commit +f, fixup = like \"squash\", but discard this commit's log message +x, exec = run command (the rest of the line) using shell +d, drop = remove commit These lines can be re-ordered; they are executed from top to bottom. " | git stripspace --comment-lines >>"$todo" -- 2.17.0.windows.1.4.g7e4058d72e3
[PATCH v7 01/17] sequencer: avoid using errno clobbered by rollback_lock_file()
As pointed out in a review of the `--rebase-merges` patch series, `rollback_lock_file()` clobbers errno. Therefore, we have to report the error message that uses errno before calling said function. Signed-off-by: Johannes Schindelin --- sequencer.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 667f35ebdff..096e6d241e0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -345,12 +345,14 @@ static int write_message(const void *buf, size_t len, const char *filename, if (msg_fd < 0) return error_errno(_("could not lock '%s'"), filename); if (write_in_full(msg_fd, buf, len) < 0) { + error_errno(_("could not write to '%s'"), filename); rollback_lock_file(&msg_file); - return error_errno(_("could not write to '%s'"), filename); + return -1; } if (append_eol && write(msg_fd, "\n", 1) < 0) { + error_errno(_("could not write eol to '%s'"), filename); rollback_lock_file(&msg_file); - return error_errno(_("could not write eol to '%s'"), filename); + return -1; } if (commit_lock_file(&msg_file) < 0) return error(_("failed to finalize '%s'"), filename); @@ -2119,9 +2121,9 @@ static int save_head(const char *head) written = write_in_full(fd, buf.buf, buf.len); strbuf_release(&buf); if (written < 0) { + error_errno(_("could not write to '%s'"), git_path_head_file()); rollback_lock_file(&head_lock); - return error_errno(_("could not write to '%s'"), - git_path_head_file()); + return -1; } if (commit_lock_file(&head_lock) < 0) return error(_("failed to finalize '%s'"), git_path_head_file()); -- 2.17.0.windows.1.4.g7e4058d72e3
[PATCH v7 00/17] rebase -i: offer to recreate commit topology
Once upon a time, I dreamt of an interactive rebase that would not flatten branch structure, but instead recreate the commit topology faithfully. My original attempt was --preserve-merges, but that design was so limited that I did not even enable it in interactive mode. Subsequently, it *was* enabled in interactive mode, with the predictable consequences: as the --preserve-merges design does not allow for specifying the parents of merge commits explicitly, all the new commits' parents are defined *implicitly* by the previous commit history, and hence it is *not possible to even reorder commits*. This design flaw cannot be fixed. Not without a complete re-design, at least. This patch series offers such a re-design. Think of --rebase-merges as "--preserve-merges done right". It introduces new verbs for the todo list, `label`, `reset` and `merge`. For a commit topology like this: A - B - C \ / D the generated todo list would look like this: # branch D pick 0123 A label branch-point pick 1234 D label D reset branch-point pick 2345 B merge -C 3456 D # C There are more patches in the pipeline, based on this patch series, but left for later in the interest of reviewable patch series: one mini series to use the sequencer even for `git rebase -i --root`, and another one to add support for octopus merges to --rebase-merges. And then one to allow for rebasing merge commits in a smarter way (this one will need a bit more work, though, as it can result in very complicated, nested merge conflicts *very* easily). Changes since v6: - Reworded the REBASING MERGES section of the man page a bit (thanks, Martin & Phillip!). - The `reset` todo command now refuses to overwrite untracked files (thanks Phillip!). - The do_merge() function was prevented from leaking memory left and right. - Added a nice advice for the case when todo commands were rescheduled. - Refactored the way we get to the original line of any given todo command in the todo list, simplifying even existing code to make it a lot more readable. - Failed `label` and `reset` commands, as well as `merge` that failed before even attempting to merge, are now rescheduled automatically (thanks Phillip and Philip!). - The do_merge() function no longer tries to commit when there are merge conflicts (thanks Phillip!). - When do_merge() failed to run the recursive merge, it no longer claims that there were conflicts (thanks Phillip!). - When the merge failed, we now write out the index before giving `rerere` a chance (d'oh!). Johannes Schindelin (15): sequencer: avoid using errno clobbered by rollback_lock_file() sequencer: make rearrange_squash() a bit more obvious sequencer: refactor how original todo list lines are accessed sequencer: offer helpful advice when a command was rescheduled sequencer: introduce new commands to reset the revision # This is a combination of 2 commits. # This is the 1st commit message: sequencer: fast-forward `merge` commands, if possible rebase-helper --make-script: introduce a flag to rebase merges rebase: introduce the --rebase-merges option sequencer: make refs generated by the `label` command worktree-local sequencer: handle post-rewrite for merge commands rebase --rebase-merges: avoid "empty merges" pull: accept --rebase=merges to recreate the branch topology rebase -i: introduce --rebase-merges=[no-]rebase-cousins rebase -i --rebase-merges: add a section to the man page Phillip Wood (1): rebase --rebase-merges: add test for --keep-empty Stefan Beller (1): git-rebase--interactive: clarify arguments Documentation/config.txt | 8 + Documentation/git-pull.txt | 5 +- Documentation/git-rebase.txt | 147 - builtin/pull.c | 14 +- builtin/rebase--helper.c | 13 +- builtin/remote.c | 18 +- contrib/completion/git-completion.bash | 4 +- git-rebase--interactive.sh | 22 +- git-rebase.sh | 16 + refs.c | 3 +- sequencer.c| 869 +++-- sequencer.h| 7 + t/t3421-rebase-topology-linear.sh | 1 + t/t3430-rebase-merges.sh | 221 +++ 14 files changed, 1288 insertions(+), 60 deletions(-) create mode 100755 t/t3430-rebase-merges.sh base-commit: fe0a9eaf31dd0c349ae4308498c33a5c3794b293 Published-As: https://github.com/dscho/git/releases/tag/recreate-merges-v7 Fetch-It-Via: git fetch https://github.com/dscho/git recreate-merges-v7 Interdiff vs v6: diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index be946de2efb..0ff83b62821 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -849,14 +849,18 @@ merge -C
Re: [PATCH] git-send-email: Cc more people
On Thu, Apr 19, 2018 at 06:21:42AM +0900, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > > > But IMO this patch is really lacking a few things before being ready: > > > > 1. You have no tests for this. See t/t9001-send-email.sh for examples, > > ... > > 2. Just a few lines down from your quoted hunk we have this: > > ... code about $supress_cc{} ... > >Your change should at least describe why those aren't being updated, > >but probably we should add some other command-line option for > >ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed > >--[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by > >a historical alias for --[no-]wildcard-by-cc=signed-off. > > 3. Ditto all the documentation in "man git-send-email" about > > ... > > Thanks, I agree that 2. (the lack of suppression) is a showstopper. I agree with that (and the lack of tests, obviously) > I'd further say that these new CC-sources should be disabled by > default and made opt-in to avoid surprising existing users. But I disagree with this. The current behaviour is surprising to existing users, to the point where people are writing their own scripts to replace git send-email (which seems crazy to me). > One thing we also need to be very careful about is that some of the > fields may not even have an e-mail address. We can expect that > S-o-b and Cc would be of form "human readable name " > by their nature, but it is perfectly fine to write only human > readable name without address on random lines like "suggeted-by" and > "helped-by". There needs a way for the end-user to avoid using data > found on such lines as if they are valid e-mail addresses. I also agree with this. I'll add some test-cases and make sure we only add these if they're valid email addresses.