Split fetch and merge logic
cover Split fetch and merge logic
This series implements the split of the fetch and merge logic.
The first patch adds tests for the merge logic. It is very extensive and expensive (in time), so I just post this to show that (almost) nothing changes (although some part of if could be included). And the third patch shows what changes.
Santi Béjar (3):
t/t5515-fetch-merge-logic.sh: Added tests for the merge login in git-fetch
git-fetch: Split fetch and merge logic
t/t5515: fixes for the separate fetch and merge logic
Quoted 1 line · Santi Béjar
This series implements the split of the fetch and merge logic.
That's a description of what it does, but could you describe what problem it solves, please?
The approach of presentation to introduce the test and showing the behavior change in a later patch is very good, though.
Quoted 6 lines · Junio C Hamano
Quoted 1 line · Santi Béjar
This series implements the split of the fetch and merge logic.
That's a description of what it does, but could you describe what problem it solves, please?
Sure.
In the past we've had problems when we have changed the merge logic (as shows 4363dfbe3). This patch makes the two process completely independent, and concentrate the merge logic in one place (leaving git-parse-remote.sh independent of the merge logic).
Quoted 3 lines
The approach of presentation to introduce the test and showing the behavior change in a later patch is very good, though.
Thanks.
Santi
Quoted 4 lines · Santi Béjar
In the past we've had problems when we have changed the merge logic (as shows
4363dfbe3). This patch makes the two process completely independent, and concentrate the merge logic in one place (leavinggit-parse-remote.shindependent of the merge logic).
But that is a solved problem, isn't it? What else does it solve? The justification for moving around the logic could be something like "these three patches do not do that themselves, but it opens a door for further work such as ...", but without something concrete in "..." part, your response makes the patch look mostly needless code churn.
I was hoping to hear something like "now git-fetch has to do much less than before, eventual C-rewrite of the command, which can borrow some code already written for 'pu' branch, will become much easier" ;-).
Quoted 14 lines · Junio C Hamano
Quoted 4 lines · Santi Béjar
In the past we've had problems when we have changed the merge logic (as shows
4363dfbe3). This patch makes the two process completely independent, and concentrate the merge logic in one place (leavinggit-parse-remote.shindependent of the merge logic).But that is a solved problem, isn't it? What else does it solve? The justification for moving around the logic could be something like "these three patches do not do that themselves, but it opens a door for further work such as ...", but without something concrete in "..." part, your response makes the patch look mostly needless code churn.
Quoted 4 lines
I was hoping to hear something like "now git-fetch has to do much less than before, eventual C-rewrite of the command, which can borrow some code already written for 'pu' branch, will become much easier" ;-).
This sounds like a good justification, also :-)
Santi
Quoted 1 line · Santi Béjar
This sounds like a good justification, also :-)
What I said was an example. I do not think your code churning actually would make it easier.
Quoted 18 lines · Junio C Hamano
Quoted 4 lines · Santi Béjar
In the past we've had problems when we have changed the merge logic (as shows
4363dfbe3). This patch makes the two process completely independent, and concentrate the merge logic in one place (leavinggit-parse-remote.shindependent of the merge logic).But that is a solved problem, isn't it? What else does it solve? The justification for moving around the logic could be something like "these three patches do not do that themselves, but it opens a door for further work such as ...", but without something concrete in "..." part, your response makes the patch look mostly needless code churn.
I was hoping to hear something like "now git-fetch has to do much less than before, eventual C-rewrite of the command, which can borrow some code already written for 'pu' branch, will become much easier" ;-).
Quoted 8 lines · Junio C Hamano
Quoted 1 line · Santi Béjar
This sounds like a good justification, also :-)
What I said was an example. I do not think your code churning actually would make it easier.
What I said was an example of why having the two logic intermixed can be problematic. Anyway, basically:
* It is a cleanup
* It makes the code cleaner
* It makes the code easier to maintain
* It makes the future changes easier
* It makes git-parse-remote and almost all git-fetch independent of the merge logic
* The merge logic is in a few lines
* Do one thing and do it well
Regards,
Santi
PATCH 2/3 git-fetch: Split fetch and merge logic
In reply to Santi Béjargit-fetch fetches the branches from the remote and saves this information in .git/FETCH_FETCHED, and at the end it generates the file .git/FETCH_HEAD.
There are two cases where the behaviour is changed:
1) branch.*.merge no longer must exactly match the remote part of the branch fetched. Both are expanded in full (as refs/heads/...) and matched afterwards.
2) When the remote is specified with $GIT_DIR/branches/... and there is a branch.*.merge, the remote branch name must match to get them merged. Before the branch in $GIT_DIR/branches/... was always merged. In the documentation the $GIT_DIR/branches/ is documented as a short-hand for a corresponding file in $GIT_DIR/remotes/, so I think this makes the new behaviour consistent.
--- Documentation/config.txt | 2 +- git-fetch.sh | 74 ++++++++++++++++++++++++++------------------- git-parse-remote.sh | 60 +++++++++---------------------------- t/t5510-fetch.sh | 16 ++++++++++ 4 files changed, 75 insertions(+), 77 deletions(-)
Documentation/config.txt +1 -1
@@ -238,7 +238,7 @@ branch.<name>.remote:: branch.<name>.merge:: When in branch <name>, it tells `git fetch` the default refspec to - be marked for merging in FETCH_HEAD. The value has exactly to match + be marked for merging in FETCH_HEAD. The value has to match a remote part of one of the refspecs which are fetched from the remote given by "branch.<name>.remote". The merge information is used by `git pull` (which at first calls
git-fetch.sh +43 -31
142 lines not on this page · raw message
git-parse-remote.sh +15 -45
103 lines not on this page · raw message
t/t5510-fetch.sh +16 -0
@@ -50,6 +50,22 @@ test_expect_success "fetch test" ' test "z$mine" = "z$his" ' +test_expect_success "fetch test fetched" ' + cd "$D" && + cd three && + git fetch && + test -f .git/refs/heads/two && + test -f .git/refs/heads/one && + master_in_two=`cd ../two && git rev-parse master` && + one_in_two=`cd ../two && git rev-parse one` && + { + echo "$master_in_two refs/heads/master:refs/heads/two" + echo "$one_in_two refs/heads/one:refs/heads/one" + } >expected && + cut -f -2 .git/FETCH_FETCHED >actual && + diff expected actual' + + test_expect_success "fetch test for-merge" ' cd "$D" && cd three &&
Signature · 1 line
1.5.0.35.gaaba
Re: [PATCH/RFC 2/3] git-fetch: Split fetch and merge logic
Quoted 9 lines · Santi Béjar
git-fetch fetches the branches from the remote and saves this information in .git/FETCH_FETCHED, and at the end it generates the file .git/FETCH_HEAD.
There are two cases where the behaviour is changed:
1) branch.*.merge no longer must exactly match the remote part of the branch fetched. Both are expanded in full (as refs/heads/...) and matched afterwards.
How hard would it be to fix this? I see this as a regression. If you are setting configuration, wouldn't you rather see the behaviour consistent even when remote adds new refs?
Quoted 3 lines
2) When the remote is specified with $GIT_DIR/branches/... and there is a branch.*.merge, the remote branch name must match to get them merged. Before the branch in $GIT_DIR/branches/... was always merged.
I do not think the current $GIT_DIR/branches/ support with respect to choosing which remote branch to choose was done with any deep thinking, other than to stay backward compatible, so I would not put too much trust in what is in the documentation. At the same time, I personally can be pursuaded to go either way, exactly because I do not think the current behaviour has strict reasoning behind it.
However, I wonder how this change would affect existing setups people may have.
Merging this at this moment would be a pain even if there were no downsides, as there are a few topics that want to touch parse-remote and fetch (two already in 'pu', and git-bundle series also wants to hook into git-fetch); these topics would need to get adjusted if this clean-up goes in first.
Re: [PATCH/RFC 2/3] git-fetch: Split fetch and merge logic
Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Santi Béjar
git-fetch fetches the branches from the remote and saves this information in .git/FETCH_FETCHED, and at the end it generates the file .git/FETCH_HEAD.
Quoted 7 lines
Quoted 5 lines
There are two cases where the behaviour is changed:
1) branch.*.merge no longer must exactly match the remote part of the branch fetched. Both are expanded in full (as refs/heads/...) and matched afterwards.
How hard would it be to fix this?
At least it is not a 2 lines "fix".
Quoted 3 lines
I see this as a regression.
If you are setting configuration, wouldn't you rather see the behaviour consistent even when remote adds new refs?
I don't see it a regression. All current setups continue to work properly and, sorry but I don't see how adding a new ref changes this.
Quoted 5 lines
Quoted 4 lines
2) When the remote is specified with $GIT_DIR/branches/... and there is a branch.*.merge, the remote branch name must match to get them merged. Before the branch in $GIT_DIR/branches/... was always merged.
Quoted 7 lines
I do not think the current $GIT_DIR/branches/ support with respect to choosing which remote branch to choose was done with any deep thinking, other than to stay backward compatible, so I would not put too much trust in what is in the documentation. At the same time, I personally can be pursuaded to go either way, exactly because I do not think the current behaviour has strict reasoning behind it.
I prefer my way, but I don't mind much either and it can be "fixed".
Quoted 9 lines
However, I wonder how this change would affect existing setups people may have.
Merging this at this moment would be a pain even if there were no downsides, as there are a few topics that want to touch parse-remote and fetch (two already in 'pu', and git-bundle series also wants to hook into git-fetch); these topics would need to get adjusted if this clean-up goes in first.
A problematic decision :)
Santi
Re: [PATCH/RFC 2/3] git-fetch: Split fetch and merge logic
Quoted 9 lines · Santi Béjar
Quoted 5 lines
Quoted 5 lines
There are two cases where the behaviour is changed:
1) branch.*.merge no longer must exactly match the remote part of the branch fetched. Both are expanded in full (as refs/heads/...) and matched afterwards.
...
Quoted 3 lines
I see this as a regression.
If you are setting configuration, wouldn't you rather see the behaviour consistent even when remote adds new refs?
Maybe I misread your description, but I took it to mean that you are allowing:
branch.master.merge = a
to mean what we traditionally spelled
branch.master.merge = refs/heads/a
and guessed (I haven't looked for where it happens in the code) the way you do that conversion is by tail-matching the ref; if the other end creates "refs/heads/b/a", suddenly remote branch b/a starts matching that pattern wouldn't it?
Earlier we fixed the ambiguous use of branch.*.merge in 756373da; I think the same reasoning should apply here.
Configuration is something you set once because you want to forget about it afterwards (iow, not having to type every time), and I think making sure it names things unambiguously outweighs one-time convenience of being able to write the configuration in a looser fashion.
If somebody does "git checkout -B origin/next" which does:
git checkout -b next origin/next &&
git repo-config branch.next.merge $mergeI would expect that the enhanced "checkout" script would not have any trouble arranging $merge to fully spell out refs/heads/next.
Quoted 7 lines
Quoted 5 lines
Merging this at this moment would be a pain even if there were no downsides, as there are a few topics that want to touch parse-remote and fetch (two already in 'pu', and git-bundle series also wants to hook into git-fetch); these topics would need to get adjusted if this clean-up goes in first.
A problematic decision :)
Not at all.
Re: [PATCH/RFC 2/3] git-fetch: Split fetch and merge logic
Quoted 17 lines · Junio C Hamano
Quoted 9 lines · Santi Béjar
Quoted 5 lines
Quoted 5 lines
There are two cases where the behaviour is changed:
1) branch.*.merge no longer must exactly match the remote part of the branch fetched. Both are expanded in full (as refs/heads/...) and matched afterwards.
...
Quoted 3 lines
I see this as a regression.
If you are setting configuration, wouldn't you rather see the behaviour consistent even when remote adds new refs?Maybe I misread your description, but I took it to mean that you are allowing:
branch.master.merge = a
Quoted 8 lines
to mean what we traditionally spelled
branch.master.merge = refs/heads/a
and guessed (I haven't looked for where it happens in the code) the way you do that conversion is by tail-matching the ref; if the other end creates "refs/heads/b/a", suddenly remote branch b/a starts matching that pattern wouldn't it?
No. branch.master.merge = a is equivalent to refs/heads/a and only matches with the remote branch refs/heads/a. It continues to exactly match the two branches, but with the full patch (refs/...). So now it is possible to have:
[remote "origin"]
url = ...
fetch = refs/heads/*:refs/heads/origin/*
[branch "master"]
remote = origin
merge = master
or the other way:
[remote "origin"]
url = ...
fetch = master:refs/heads/origin
[branch "master"]
remote = origin
merge = refs/heads/master
Quoted 9 lines
Earlier we fixed the ambiguous use of branch.*.merge in
756373da; I think the same reasoning should apply here.Configuration is something you set once because you want to forget about it afterwards (iow, not having to type every time), and I think making sure it names things unambiguously outweighs one-time convenience of being able to write the configuration in a looser fashion.
It is unambiguous.
But if it is problematic I'll try to keep the current behaviour.
Santi
PATCH 3/3 t/t5515: fixes for the separate fetch and merge logic
In reply to Santi Béjar1) branch.*.merge no longer must exactly match the remote part of the branch fetched. Both are expanded in full (as refs/heads/...) and matched afterwards.
2) When the remote is specified with $GIT_DIR/branches/... and there is a branch.*.merge, the remote branch name must match to get them merged. Before the branch in $GIT_DIR/branches/... was always merged. In the documentation the $GIT_DIR/branches/ is documented as a short-hand for a corresponding file in $GIT_DIR/remotes/, so I think this makes the new behaviour consistent.
--- t/t5515/fetch.br-branches-default-merge | 2 +- ...etch.br-branches-default-merge_branches-default | 2 +- t/t5515/fetch.br-branches-default-octopus | 2 +- ...ch.br-branches-default-octopus_branches-default | 2 +- t/t5515/fetch.br-branches-one-merge | 2 +- t/t5515/fetch.br-branches-one-merge_branches-one | 2 +- t/t5515/fetch.br-config-glob-octopus | 2 +- t/t5515/fetch.br-config-glob-octopus_config-glob | 2 +- t/t5515/fetch.br-remote-glob-octopus | 2 +- t/t5515/fetch.br-remote-glob-octopus_remote-glob | 2 +- 10 files changed, 10 insertions(+), 10 deletions(-)
t/t5515/fetch.br-branches-default-merge +1 -1
@@ -1,2 +1,2 @@ # br-branches-default-merge -754b754407bf032e9a2f9d5a9ad05ca79a6b228f +754b754407bf032e9a2f9d5a9ad05ca79a6b228f not-for-merge
t/t5515/fetch.br-branches-default-merge_branches-default +1 -1
@@ -1,2 +1,2 @@ # br-branches-default-merge branches-default -754b754407bf032e9a2f9d5a9ad05ca79a6b228f +754b754407bf032e9a2f9d5a9ad05ca79a6b228f not-for-merge
t/t5515/fetch.br-branches-default-octopus +1 -1
@@ -1,2 +1,2 @@ # br-branches-default-octopus -754b754407bf032e9a2f9d5a9ad05ca79a6b228f +754b754407bf032e9a2f9d5a9ad05ca79a6b228f not-for-merge
t/t5515/fetch.br-branches-default-octopus_branches-default +1 -1
@@ -1,2 +1,2 @@ # br-branches-default-octopus branches-default -754b754407bf032e9a2f9d5a9ad05ca79a6b228f +754b754407bf032e9a2f9d5a9ad05ca79a6b228f not-for-merge
t/t5515/fetch.br-branches-one-merge +1 -1
@@ -1,2 +1,2 @@ # br-branches-one-merge -8e32a6d901327a23ef831511badce7bf3bf46689 +8e32a6d901327a23ef831511badce7bf3bf46689 not-for-merge
t/t5515/fetch.br-branches-one-merge_branches-one +1 -1
@@ -1,2 +1,2 @@ # br-branches-one-merge branches-one -8e32a6d901327a23ef831511badce7bf3bf46689 +8e32a6d901327a23ef831511badce7bf3bf46689 not-for-merge
t/t5515/fetch.br-config-glob-octopus +1 -1
@@ -2,4 +2,4 @@ 754b754407bf032e9a2f9d5a9ad05ca79a6b228f not-for-merge 8e32a6d901327a23ef831511badce7bf3bf46689 0567da4d5edd2ff4bb292a465ba9e64dcad9536b not-for-merge -6134ee8f857693b96ff1cc98d3e2fd62b199e5a8 not-for-merge +6134ee8f857693b96ff1cc98d3e2fd62b199e5a8
t/t5515/fetch.br-config-glob-octopus_config-glob +1 -1
@@ -2,4 +2,4 @@ 754b754407bf032e9a2f9d5a9ad05ca79a6b228f not-for-merge 8e32a6d901327a23ef831511badce7bf3bf46689 0567da4d5edd2ff4bb292a465ba9e64dcad9536b not-for-merge -6134ee8f857693b96ff1cc98d3e2fd62b199e5a8 not-for-merge +6134ee8f857693b96ff1cc98d3e2fd62b199e5a8
t/t5515/fetch.br-remote-glob-octopus +1 -1
@@ -2,4 +2,4 @@ 754b754407bf032e9a2f9d5a9ad05ca79a6b228f not-for-merge 8e32a6d901327a23ef831511badce7bf3bf46689 0567da4d5edd2ff4bb292a465ba9e64dcad9536b not-for-merge -6134ee8f857693b96ff1cc98d3e2fd62b199e5a8 not-for-merge +6134ee8f857693b96ff1cc98d3e2fd62b199e5a8
t/t5515/fetch.br-remote-glob-octopus_remote-glob +1 -1
@@ -2,4 +2,4 @@ 754b754407bf032e9a2f9d5a9ad05ca79a6b228f not-for-merge 8e32a6d901327a23ef831511badce7bf3bf46689 0567da4d5edd2ff4bb292a465ba9e64dcad9536b not-for-merge -6134ee8f857693b96ff1cc98d3e2fd62b199e5a8 not-for-merge +6134ee8f857693b96ff1cc98d3e2fd62b199e5a8
Signature · 1 line
1.5.0.35.gaaba
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH/RFC 1/3] t/t5515-fetch-merge-logic.sh: Added tests for the merge login in git-fetch
Hi *,
I think it didn't get to the list due to the size limit. So I send it as a gziped attatchment, with the test inline.
Regards,
Santi
--- t/t5515-fetch-merge-logic.sh | 124 ++++++++++++++++++++
t/t5515-fetch-merge-logic.sh new file +124 -0
@@ -0,0 +1,124 @@ +#!/bin/sh +# +# Copyright (c) 2007 Santi Béjar, based on t4013 by Junio C Hamano +# +# + +test_description='Merge logic in fetch' + +. ./test-lib.sh + +LF=' +' + +test_expect_success setup ' + GIT_AUTHOR_DATE="2006-06-26 00:00:00 +0000" && + GIT_COMMITTER_DATE="2006-06-26 00:00:00 +0000" && + export GIT_AUTHOR_DATE GIT_COMMITTER_DATE && + + echo >file original && + git add file && + git commit -a -m One && + git branch one && + + echo two >> file && + git commit -a -m Two && + git branch two && + + echo three >> file && + git commit -a -m Three && + git branch three && + + echo master >> file && + git commit -a -m Master + + git checkout three + + git clone . cloned && + cd cloned && + + git config remote.config-explicit.url ../.git/ && + git config remote.config-explicit.fetch refs/heads/master:remotes/rem/master && + git config --add remote.config-explicit.fetch refs/heads/one:remotes/rem/one && + git config --add remote.config-explicit.fetch two:remotes/rem/two && + git config --add remote.config-explicit.fetch refs/heads/three:remotes/rem/three && + + git config remote.config-glob.url ../.git/ && + git config remote.config-glob.fetch refs/heads/*:refs/remotes/rem/* && + + mkdir -p .git/remotes && + { + echo "URL: ../.git/" + echo "Pull: refs/heads/master:remotes/rem/master" + echo "Pull: refs/heads/one:remotes/rem/one" + echo "Pull: two:remotes/rem/two" + echo "Pull: refs/heads/three:remotes/rem/three" + } >.git/remotes/remote-explicit && + + { + echo "URL: ../.git/" + echo "Pull: refs/heads/*:refs/remotes/rem/*" + } >.git/remotes/remote-glob && + + mkdir -p .git/branches && + echo "../.git" > .git/branches/branches-default && + echo "../.git#one" > .git/branches/branches-one && + + for remote in config-explicit config-glob remote-explicit remote-glob \ + branches-default branches-one ; do + git config branch.br-$remote.remote $remote && + git config branch.br-$remote-merge.remote $remote && + git config branch.br-$remote-merge.merge refs/heads/three && + git config branch.br-$remote-octopus.remote $remote && + git config branch.br-$remote-octopus.merge refs/heads/one && + git config --add branch.br-$remote-octopus.merge two && + git config --add branch.br-$remote-octopus.merge remotes/rem/three && + branches="$branches br-$remote br-$remote-merge br-$remote-octopus" + done +' + +for branch in br-unconfig $branches ; do + echo $branch + for remote in config-explicit config-glob remote-explicit remote-glob \ + branches-default branches-one ../.git + do +cat <<EOF +$branch $remote +$branch $remote one +$branch $remote one two +EOF + done +done > tests + +while read cmd +do + case "$cmd" in + '' | '#'*) continue ;; + esac + test=`echo "$cmd" | sed -e 's|[/ ][/ ]*|_|g'` + cnt=`expr $test_count + 1` + pfx=`printf "%04d" $cnt` + expect="../../t5515/fetch.$test" + actual="$pfx-fetch.$test" + + test_expect_success "$cmd" ' + { + echo "# $cmd" + set x $cmd; shift + git symbolic-ref HEAD refs/heads/$1 ; shift + git fetch "$@" >/dev/null + cut -f -2 .git/FETCH_HEAD + } >"$actual" && + if test -f "$expect" + then + diff -u "$expect" "$actual" && + rm -f "$actual" + else + # this is to help developing new tests. + cp "$actual" "$expect" + false + fi + ' +done < tests + +test_done