Re: `--rebase-merges' still failing badly
On Thu, 11 Oct 2018 08:01:40 +0900, Junio wrote: > Michael Witten writes: > >> On Wed, 10 Oct 2018 14:43:46 +0900, Junio wrote: >> >>> We haven't seen much complaints and breakages reported against the >>> two big "rewrite in C" topics around "rebase"; perhaps it is a good >>> time to merge them to 'next' soonish to cook them for a few weeks >>> before moving them to 'master'? >> >> In my opinion, the `--rebase-merges' feature has been broken since the >> beginning, and the builtin version should be fixed before it is moved >> ahead. > > [...] > > If "rebase-merges" has been broken since the beginning, as long as the > "rewrite in C" topics around "rebase" do not make it even worse, I do > not think it is a good move to block the topics moving forward. If the > feature were so broken that it is not practically useful, then people > wouldn't be using it in the versions of Git before the rewrite, so it > won't harm anybody if the same feature in the rewritten version is > equally (or even more severely) broken, as long as the other parts of > the feature works at least equally well compared to the older version. > > We are not in the business of hostage taking. > > What *should* block the rewrited version is a regression, i.e. > something that used to work well no longer works or works differently > in such a way that established workflows need to be adjusted. > > [...] I do not think that is a reason to keep "rewrite in C" waiting in > 'pu'. * Your logic is appealing, and I nearly pursuaded myself by the same reasoning to submit my email as a separate discussion, as you suggest. However, what convinced me otherwise is the following: The closer you move the rewrite to a fast-forward-only public branch name, the more likely downstream projects are going to set up new, long-lived releases around this very useful but nevertheless broken feature. The moment you announce a new release, there are going to be a bunch of people who grab that release and then NEVER look back, and so the rest of us will be stuck with this problem for who knows how long. So, not only is this an appeal to the authors to fix this problem, but its also an appeal to you to make sure that the next major release includes the fix. * Also, I say the following without irony or tongue in cheek: Maybe, no one has complained because few people are using this feature yet, or their commit summaries are simplistic, or they've got workarounds (as I've got). Not only must this feature be turned on explicitly, but `git' has existed for over a decade *without* it; users who are interested in sophisticated management of commit history have already developed other ways to achieve the same result (I know I did), or their commit messages are so simplistic that the bug is never triggered, or they just plan around it by automatically running a quick search/replace for the offending characters or for the irritating "labels". If the last decade has shown us anything, it's that git's fundamentals are so good that programmers can get around any bug on their own, without having to appeal to others for help. And, what is a programmer if not someone who is used to making things Just Work [Damnit]? As an illustration, consider the recent `break' command that is being added to the repertoire of `git rebase -i'. Hell, I (and probably many others) have been doing that for YEARS with: x false No need for a "new" command. I bet that 10 years from now, people will *still* be using their own ways, and will *still* be totally oblivious to the existence of `break'. That is to say, I wouldn't put much faith in the degree to which people report issues. The programming world has a lot of itchy backs, and just as many personal inventions for scratching them. As always, thanks for taking the time to review everyone's input. Sincerely, Michael Witten
Re: `--rebase-merges' still failing badly
On Wed, 10 Oct 2018 18:51:17 -, Michael Witten wrote: > merge -# Same as merge -C abcde r1 That should be: merge -# Same as `merge r1'
`--rebase-merges' still failing badly
On Wed, 10 Oct 2018 14:43:46 +0900, Junio wrote:
> We haven't seen much complaints and breakages reported against the
> two big "rewrite in C" topics around "rebase"; perhaps it is a good
> time to merge them to 'next' soonish to cook them for a few weeks
> before moving them to 'master'?
In my opinion, the `--rebase-merges' feature has been broken since the
beginning, and the builtin version should be fixed before it is moved
ahead. In short: "labels" are brittle; see below for tests.
Also, here are some quick *additional* thoughts:
* Labels should be simply "r0", "r1", ... "rN".
* The current, long label names are just cumbersome.
* The embedded comments are already more than enough.
* "r" is short for "revision" or "reset" or "remember", etc.
* "r" is located on a QWERTY keyboard such that it's very
easy to type "rN", where "N" is a number.
* Why is the command "label" and not "branch"? Every other related
command looks like a normal git command: "reset" and "merge".
Make it "branch".
* In my experience, there's a lot of this boiler plate:
pick 12345
label r1
reset r0
merge r1
How about instead, use git's existing ideas:
pick 12345
reset r0
merge ORIG_HEAD
Or, maybe git in general should treat `-' as `ORIG_HEAD' (which
would be similar to how `git checkout' understands `-'), thereby
allowing a very quick idiomatic string of commands:
pick 12345
reset r0
merge -
In truth, I don't really know the semantics of `ORIG_HEAD', so
maybe those should be nailed down and documented more clearly;
I would like it to work as in the following:
pick 12345
# label r1 (pretend)
reset r0 # Store r1 in ORIG_HEAD
pick 67890 # Do NOT touch ORIG_HEAD
merge -# Same as merge -C abcde r1
Anyway, this kind of unspoken behavior would make *writing* a
new history by hand much more pleasant.
* Why not just `--merges' instead of `--rebase-merges'? Or, better
yet, just make it the default behavior; the special option
should instead be:
--flatten
This option would simply tell `git rebase' to prepare an initial
todo list without merges.
Thanks for this great feature.
I'm only complaining so much because it's such a useful feature, and I
want it to be even better, because I'll probably use it A LOT; it
should have been available since the start as a natural consequence of
the way git works.
Sincerely,
Michael Witten
---
Unfortunately, both the legacy version and the rewrite of
`--rebase-merges' display a bug that makes this feature fairly
unusable in practice; it tries to create a "label" (i.e., a branch
name) from a commit log summary line, and the result is often invalid
(or just plain irritating to work with). In particular, it fails on
typical characters, including at least these:
:/\?.*[]
To see this, first define some POSIX shell functions:
test()
{
(
set -e
summary=$1
d=/tmp/repo # WARNING. CHANGE IF NECESSARY.
rm -rf "$d"; mkdir -p "$d"; cd "$d"
git init -q
echo a > a; git add a; git commit -q -m a
git branch base
echo b > b; git add b; git commit -q -m b
git reset -q --hard HEAD^
git merge -q --no-ff -m "$summary" ORIG_HEAD
git log --graph --oneline
git rebase --rebase-merges base
); status=$?
echo
return "$status"
}
Test()
{
if test "$@" 1>/dev/null 2>&1; then
echo 'good'; return 0
else
echo 'fail'; return 1
fi
}
Then, try various commit summaries (see below for results):
test c
test 'combine these into a merge: a and b'
Test ab:
Test a:b
Test :
Test a/b
Test 'Now supports /regex/'
Test ab/
Test /ab
Test /
Test 'a\b'
Test '\'
Test 'Maybe this works?'
Test '?'
Test 'This does not work.'
Test 'This works. Strange!'
Test .git
Test .
Test 'Cast each pointer to *void'
Test '*'
Test 'return a[1] not a[0]'
Test '[ does not work'
Test '['
Test '] does work'
Test ']'
Here are the results of pasting the above commands into my terminal:
$ test c
warning: templates not found in ../install/share/git-core/templates
* 1992d07 (HEAD -> master) c
|\
| * 34555
[PATCH] docs: typo: s/isimilar/similar/
Signed-off-by: Michael Witten --- Documentation/git-rebase.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 1fbc6ebcde..432baabe33 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -954,7 +954,7 @@ command fails, it is rescheduled immediately, with a helpful message how to proceed. The `reset` command resets the HEAD, index and worktree to the specified -revision. It is isimilar to an `exec git reset --hard `, but +revision. It is similar to an `exec git reset --hard `, but refuses to overwrite untracked files. If the `reset` command fails, it is rescheduled immediately, with a helpful message how to edit the todo list (this typically happens when a `reset` command was inserted into the todo -- 2.18.0
[PATCH] docs: graph: Remove unnecessary `graph_update()' call
The sample code calls `get_revision()' followed by `graph_update()',
but the documentation and source code indicate that `get_revision()'
already calls `graph_update()' for you.
Signed-off-by: Michael Witten
---
Documentation/technical/api-history-graph.txt | 1 -
1 file changed, 1 deletion(-)
diff --git a/Documentation/technical/api-history-graph.txt
b/Documentation/technical/api-history-graph.txt
index 18142b6d29..d9fd98d435 100644
--- a/Documentation/technical/api-history-graph.txt
+++ b/Documentation/technical/api-history-graph.txt
@@ -115,7 +115,6 @@ struct commit *commit;
struct git_graph *graph = graph_init(opts);
while ((commit = get_revision(opts)) != NULL) {
- graph_update(graph, commit);
while (!graph_is_commit_finished(graph))
{
struct strbuf sb;
--
2.18.0
[PATCH] docs: typo: s/go/to/
Signed-off-by: Michael Witten --- Documentation/technical/api-history-graph.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/technical/api-history-graph.txt b/Documentation/technical/api-history-graph.txt index 18142b6d29..729d6a0cf3 100644 --- a/Documentation/technical/api-history-graph.txt +++ b/Documentation/technical/api-history-graph.txt @@ -80,7 +80,7 @@ Calling sequence it is invoked. * For each commit, call `graph_next_line()` repeatedly, until - `graph_is_commit_finished()` returns non-zero. Each call go + `graph_is_commit_finished()` returns non-zero. Each call to `graph_next_line()` will output a single line of the graph. The resulting lines will not contain any newlines. `graph_next_line()` returns 1 if the resulting line contains the current commit, or 0 if this is merely a line -- 2.18.0
[PATCH 1/3] Testing: XDG config files: Export a suitable `XDG_CONFIG_HOME' environment variable
The tests in:
t/t1306-xdg-files.sh
were failing because the git commands were using the environment
variable `XDG_CONFIG_HOME' as it was set for the user's usual
environment, rather than as set for the testing environment.
This commit provides the quickest, simplest hack to make things work;
because there is already the setting and exporting of the environment
variable `HOME' in:
t/test-lib.sh
this commit simply adds to that file the setting and exporting of
the variable `XDG_CONFIG_HOME' (based on the variable `HOME' that is
provided there).
However, the existing tests [sometimes] don't use these variables
explicitly, so the whole structure of this testing rests on the
hope that people maintain the conventions captured by the values
of these variables; another commit should fix this instability
by using these variables explicitly.
(Note: Double quotes are not needed around the value assigned
to the variable, as word splitting is not performed).
Signed-off-by: Michael Witten mfwit...@gmail.com
---
t/test-lib.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index acda33d..69bcc75 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -544,6 +544,9 @@ rm -fr $test || {
HOME=$TRASH_DIRECTORY
export HOME
+XDG_CONFIG_HOME=$HOME/.config
+export XDG_CONFIG_HOME
+
if test -z $TEST_NO_CREATE_REPO; then
test_create_repo $test
else
--
1.7.11.1.29.gf71be5c
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] Testing: XDG config files: Use $HOME and $XDG_CONFIG_HOME explicitly
The tests in: t/t1306-xdg-files.sh relied on brittle conventions: * $HOME and $XDG_CONFIG_HOME having certain values. * The testing commands having a certain current working directory; at least one test failed as a result. This commit mitigates the problem by using the variables $HOME and $XDG_CONFIG_HOME explicitly. Signed-off-by: Michael Witten mfwit...@gmail.com --- t/t1306-xdg-files.sh | 69 +++- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh index 3c75c3f..2327047 100755 --- a/t/t1306-xdg-files.sh +++ b/t/t1306-xdg-files.sh @@ -9,58 +9,60 @@ test_description='Compatibility with $XDG_CONFIG_HOME/git/ files' . ./test-lib.sh +GIT_CONFIG_DIR=$XDG_CONFIG_HOME/git + -test_expect_success 'read config: xdg file exists and ~/.gitconfig doesn'\''t' ' +test_expect_success 'read config: xdg file exists and $HOME/.gitconfig doesn'\''t' ' - mkdir -p .config/git + mkdir -p $GIT_CONFIG_DIR - echo [alias] .config/git/config + echo [alias] $GIT_CONFIG_DIR/config - echo myalias = !echo in_config .config/git/config + echo myalias = !echo in_config $GIT_CONFIG_DIR/config echo in_config expected git myalias actual test_cmp expected actual ' -test_expect_success 'read config: xdg file exists and ~/.gitconfig exists' ' +test_expect_success 'read config: xdg file exists and $HOME/.gitconfig exists' ' - .gitconfig + $HOME/.gitconfig - echo [alias] .gitconfig + echo [alias] $HOME/.gitconfig - echo myalias = !echo in_gitconfig .gitconfig + echo myalias = !echo in_gitconfig $HOME/.gitconfig echo in_gitconfig expected git myalias actual test_cmp expected actual ' -test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''t' ' +test_expect_success 'read with --get: xdg file exists and $HOME/.gitconfig doesn'\''t' ' - rm .gitconfig + rm $HOME/.gitconfig - echo [user] .config/git/config + echo [user] $GIT_CONFIG_DIR/config - echo name = read_config .config/git/config + echo name = read_config $GIT_CONFIG_DIR/config echo read_config expected git config --get user.name actual test_cmp expected actual ' -test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' ' +test_expect_success 'read with --get: xdg file exists and $HOME/.gitconfig exists' ' - .gitconfig + $HOME/.gitconfig - echo [user] .gitconfig + echo [user] $HOME/.gitconfig - echo name = read_gitconfig .gitconfig + echo name = read_gitconfig $HOME/.gitconfig echo read_gitconfig expected git config --get user.name actual test_cmp expected actual ' -test_expect_success 'read with --list: xdg file exists and ~/.gitconfig doesn'\''t' ' +test_expect_success 'read with --list: xdg file exists and $HOME/.gitconfig doesn'\''t' ' - rm .gitconfig + rm $HOME/.gitconfig echo user.name=read_config expected git config --global --list actual test_cmp expected actual ' -test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' ' +test_expect_success 'read with --list: xdg file exists and $HOME/.gitconfig exists' ' - .gitconfig + $HOME/.gitconfig - echo [user] .gitconfig + echo [user] $HOME/.gitconfig - echo name = read_gitconfig .gitconfig + echo name = read_gitconfig $HOME/.gitconfig echo user.name=read_gitconfig expected git config --global --list actual test_cmp expected actual @@ -75,8 +77,8 @@ test_expect_success 'Setup' ' test_expect_success 'Exclusion of a file in the XDG ignore file' ' - mkdir -p $HOME/.config/git/ + mkdir -p $GIT_CONFIG_DIR - echo to_be_excluded $HOME/.config/git/ignore + echo to_be_excluded $GIT_CONFIG_DIR/ignore test_must_fail git add to_be_excluded ' @@ -89,7 +91,7 @@ test_expect_success 'Exclusion in both XDG and local ignore files' ' test_expect_success 'Exclusion in a non-XDG global ignore file' ' rm .gitignore - echo $HOME/.config/git/ignore + echo $GIT_CONFIG_DIR/ignore echo to_be_excluded $HOME/my_gitignore git config core.excludesfile $HOME/my_gitignore test_must_fail git add to_be_excluded @@ -100,7 +102,7 @@ test_expect_success 'Checking attributes in the XDG attributes file' ' echo foo f git check-attr -a f actual test_line_count -eq 0 actual - echo f attr_f $HOME/.config/git/attributes + echo f attr_f $GIT_CONFIG_DIR/attributes echo f: attr_f: set expected git check-attr -a f actual test_cmp expected actual @@ -125,18 +127,18 @@ test_expect_success 'Checking attributes
[PATCH 3/3] Testing: XDG config files: Trivial: `xdg' - `XDG'
For the sake of consistency and correctness, the acronymn `xdg' has been capitalized (`XDG'). Signed-off-by: Michael Witten mfwit...@gmail.com --- t/t1306-xdg-files.sh | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh index 2327047..6a3ab09 100755 --- a/t/t1306-xdg-files.sh +++ b/t/t1306-xdg-files.sh @@ -11,7 +11,7 @@ GIT_CONFIG_DIR=$XDG_CONFIG_HOME/git -test_expect_success 'read config: xdg file exists and $HOME/.gitconfig doesn'\''t' ' +test_expect_success 'read config: XDG file exists and $HOME/.gitconfig doesn'\''t' ' mkdir -p $GIT_CONFIG_DIR echo [alias] $GIT_CONFIG_DIR/config echo myalias = !echo in_config $GIT_CONFIG_DIR/config @@ -21,7 +21,7 @@ ' -test_expect_success 'read config: xdg file exists and $HOME/.gitconfig exists' ' +test_expect_success 'read config: XDG file exists and $HOME/.gitconfig exists' ' $HOME/.gitconfig echo [alias] $HOME/.gitconfig echo myalias = !echo in_gitconfig $HOME/.gitconfig @@ -31,7 +31,7 @@ ' -test_expect_success 'read with --get: xdg file exists and $HOME/.gitconfig doesn'\''t' ' +test_expect_success 'read with --get: XDG file exists and $HOME/.gitconfig doesn'\''t' ' rm $HOME/.gitconfig echo [user] $GIT_CONFIG_DIR/config echo name = read_config $GIT_CONFIG_DIR/config @@ -41,7 +41,7 @@ ' -test_expect_success 'read with --get: xdg file exists and $HOME/.gitconfig exists' ' +test_expect_success 'read with --get: XDG file exists and $HOME/.gitconfig exists' ' $HOME/.gitconfig echo [user] $HOME/.gitconfig echo name = read_gitconfig $HOME/.gitconfig @@ -51,7 +51,7 @@ ' -test_expect_success 'read with --list: xdg file exists and $HOME/.gitconfig doesn'\''t' ' +test_expect_success 'read with --list: XDG file exists and $HOME/.gitconfig doesn'\''t' ' rm $HOME/.gitconfig echo user.name=read_config expected git config --global --list actual @@ -59,7 +59,7 @@ ' -test_expect_success 'read with --list: xdg file exists and $HOME/.gitconfig exists' ' +test_expect_success 'read with --list: XDG file exists and $HOME/.gitconfig exists' ' $HOME/.gitconfig echo [user] $HOME/.gitconfig echo name = read_gitconfig $HOME/.gitconfig @@ -127,7 +127,7 @@ ' -test_expect_success 'write: xdg file exists and $HOME/.gitconfig doesn'\''t' ' +test_expect_success 'write: XDG file exists and $HOME/.gitconfig doesn'\''t' ' mkdir -p $GIT_CONFIG_DIR $GIT_CONFIG_DIR/config test_might_fail rm $HOME/.gitconfig @@ -138,7 +138,7 @@ ' -test_expect_success 'write: xdg file exists and $HOME/.gitconfig exists' ' +test_expect_success 'write: XDG file exists and $HOME/.gitconfig exists' ' $HOME/.gitconfig git config --global user.name write_gitconfig echo [user] expected -- 1.7.11.1.29.gf71be5c -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements
On 2011-11-04 02:11:49 GMT, Ben Walton wrote: Excerpts from Eric Wong's message of Wed Nov 02 18:09:41 -0400 2011: Hi Eric, I don't have much time to help you fix it, but I got numerous errors on SVN 1.6.x (svn 1.6.12). Can you make sure things continue to work on 1.6 and earlier, also? Yes, it's a bit of a mess, I think. It looks as though the modification required within Git::SVN::Ra is going to negatively impact other code paths that interact with that package from the outside. For example, when doing git svn init --minimize-url ..., the minimized url is not escaped while the url is. The minimized url is used to strip off the head from the full url using a regex. This now breaks because of the escaping. Fixing this locally to the use of the minimized url let me move on farther but I then got another core dump. Maybe just enable the escaping for file:// on = SVN 1.7 I think that it would be best if this change was only effective for 1.7. I wonder if all URL-ish objects should be (conditionally iff svn = 1.7) subjected to escaping? This would require some restructuring and will take me a bit of time to work out as I need to familiarize myself with the code to a deeper level. Pointers welcomed. :) This problem still exists. It should be fixed---preferably by the people who built this apparently unwieldy contraption. Sincerely, Michael Witten -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
