v2, 6 partsSupport triangular workflows

This follows-up [1], with three important differences:

1. pushremote_get() and remote_get() share code better.  Thanks Jeff.

2. All spelling mistakes have been corrected.  Thanks Eric.

3. One new test for each of the new configuration variables.  The
extra two parts [2/6] and [3/6] preprare the file for introducing
tests.  However, I've not gone overboard in this preparation; I don't
replicate the work done by Jonathan in [2].

Thanks for reading.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/218410 [2]: http://thread.gmane.org/gmane.comp.version-control.git/218451/focus=218465

Ramkumar Ramachandra (6):
  remote.c: simplify a bit of code using git_config_string()
  t5516 (fetch-push): update test description
  t5516 (fetch-push): introduce mk_test_with_name()
  remote.c: introduce a way to have different remotes for fetch/push
  remote.c: introduce remote.pushdefault
  remote.c: introduce branch.<name>.pushremote

 Documentation/config.txt | 23 +++++++++++++++---
 builtin/push.c           |  2 +-
 remote.c                 | 36 +++++++++++++++++++++------
 remote.h                 |  1 +
 t/t5516-fetch-push.sh    | 63 ++++++++++++++++++++++++++++++++++++++++--------
 5 files changed, 104 insertions(+), 21 deletions(-)

A small segment where handle_config() parses the branch.remote configuration variable can be simplified using git_config_string().

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 remote.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Ramkumar Ramachandra wrote:

Shouldn't this say

			if (git_config_string(&branch->remote_name, key, value))
				return -1;

or something?

Thanks, Jonathan

Jonathan Nieder wrote:

Yes, and so should the instances in [5/6] and [6/6].  Thanks for catching it.

The file was originally created in bcdb34f (Test wildcard push/fetch, 2007-06-08), and only contained tests that exercised wildcard functionality at the time. In subsequent commits, many other tests unrelated to wildcards were added but the test description was never updated. Fix this.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5516-fetch-push.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Ramkumar Ramachandra wrote:

I'm not thrilled with the description before or after. Would it make sense to do something like the following?

	test_description='Tests of basic fetch/push functionality.
	
	These tests create small test repositories and fetch from and
	push to them, testing:

	 * commandline syntax
	 * refspecs and default refspecs
	 * fast-forward detection and overriding fast-forward detection
	 * configuration (insteadOf, pushInsteadOf, [remote "name"] push,
	   etc)
	 * hooks
	 * --porcelain output format
	 * hiderefs
	'

Jonathan Nieder wrote:

No. When I want to add a test for branch.<name>.pushremote, I grep for branch.*.pushurl, and open files with sensible names; I'm not going to open up the file and read a long description of what tests it already contains. The filename and test headlines are sufficient. Our test suite is bad enough as it is (inconsistent style, missing &&, false positives)- I'm against adding to the maintenance burden.

Ramkumar Ramachandra wrote:

Huh? The test_description is output for "./t5516-* --help" and is supposed to help people hacking on the test to understand its setup and its purpose.

mk_test() creates a repository with the constant name "testrepo", and this may be limiting for tests that need to create more than one repository for testing. To fix this, create a new mk_test_with_name() which accepts the repository name as $1. Reimplement mk_test() as a special case of this function, making sure that no tests need to be rewritten. Do the same thing for check_push_result().

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5516-fetch-push.sh | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

Ramkumar Ramachandra wrote:

Why not give mk_test an optional parameter?

	repo_name=${1:-testrepo}

Oh, it is because mk_test already takes arguments naming refs to push. I suppose the change description could make this clearer.

Why not use mk_test and then rename, like so?

	mk_test ...refs... &&
	mv testrepo testrepo-a &&

	mk_test ...refs... &&
	mv testrepo testrepo-b &&
	...

I dunno. The helper functions at the top of this test are already intimidating, so I guess I am looking for a way to avoid making that problem worse. One way would be to add an opening comment before the function definition explaining how it is meant to be used. See t/test-lib-functions.sh for examples, such as test_cmp.

Hope that helps, Jonathan

Jonathan Nieder wrote:

Isn't it obvious?

No.  This is ugly.  mk_test() should not hardcode "testrepo".

My patch does not make the situation worse in any way: it just adds one line that passes $1 as a parameter to existing code. Yes, the functions and tests can be improved greatly, but I refrained from doing so because of your series. We can save it for later.

Ramkumar Ramachandra wrote:

[...]

Um, yes it does. It adds another function to learn to an already intimidating list.

Hoping that clarifies, Jonathan

On Wed, Mar 20, 2013 at 11:41:57AM -0700, Jonathan Nieder wrote:

Personally I do not find the set of helper functions intimidating. I tend to read the tests in a top-down manner: a test is interesting (usually because it fails), and then I want to see what it is doing, so I look at any functions it calls, and so forth.

What I usually find _much_ harder to debug is when there is hidden state leftover from other tests. So even though it is longer to write, I would much rather see:

  test_expect_success 'check that frob only affects foo' '
          set_state_of foo &&
          set_state_of bar &&
          git frob &&
          check_state_of foo &&
          check_state_of bar
  '

than for the test to assume the state of "foo" or "bar" prior to the test. And I think helper functions can help make writing those sorts of pre-conditions more reasonable (and without looking too hard, I think t5516 does an OK job of that).

Related to that is when the helper functions operate on hidden state. In this instance, we have tests that do things like:

    mk_empty &&
    git push testrepo refs/heads/master:refs/remotes/origin/master

and as a reader I say "wait, what's in testrepo?". I can follow mk_empty and see that it hardcodes testrepo, but it is even better if the function and its arguments are named in a way that I don't have to. So even though it is more typing, I would argue that:

  mk_empty testrepo &&
  git push testrepo ...

is better, because the test script is more readable as a unit.

None of this is that huge a deal to me (and yet I seem to have written a page about it :) ), but I figure while we are bikeshedding about test style...

-Peff

Jeff King <peff@peff.net> writes:

;-)  But the above is a good point.

Jeff King wrote:

Thanks for articulating this. I agree that keeping track of state is the hardest part of working with git's tests.

To clarify my earlier comment, I was reading the test script from the point of view of someone who wants to add an additional test, rather than someone debugging an existing one. That person has a difficult task: she needs to understand

 * What do the existing tests in the script do?  This information
   is important in deciding whether the new test will be redundant.

 * How do I work with the particular dialect used in the script,
   including helpers?  A new test should fit in with the style of its
   surroundings to avoid contributing to an unmaintainable mess.

 * What is the intended scope of the test script?  Does my new test
   belong in this script?

 * What is the logical progression of the script?  What story does this
   script tell?  Where should I insert my test to maintain a logical
   ordering?

 * What state that later tests rely on do I need to avoid clobbering?

Generally the best way to help such a person is to make the script very simple. In particular, using standard helpers instead of custom functions when appropriate and documenting helpers used to give new readers a quick introduction to the dialect can help a lot.

Currently, do_push() in push.c calls remote_get(), which gets the configured remote for fetching and pushing. Replace this call with a call to pushremote_get() instead, a new function that will return the remote configured specifically for pushing. This function tries to work with the string pushremote_name, before falling back to the codepath of remote_get(). This patch has no visible impact, but serves to enable future patches to introduce configuration variables to set this variable.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/push.c |  2 +-
 remote.c       | 25 +++++++++++++++++++++----
 remote.h       |  1 +
 3 files changed, 23 insertions(+), 5 deletions(-)

Ramkumar Ramachandra wrote:

The above description does not make the impact of this change clear to me. Could you give a before-and-after example? How will this internal API change make my life easier as a developer?

Hope that helps, Jonathan

Ramkumar Ramachandra <artagnon@gmail.com> writes:

The code to read branch.$name.remote configuration flips explicit_default_remote_name to one when it is used to set the default_remote_name, and that controls the value of name_given in this codepath. At this point in the series, you do not have a corresponding branch.$name.pushremote, but your [6/6] does not seem to do the same.

Why isn't it necessary to add explicit_default_pushremote_name and do the same here in patch [6/6]?

Junio C Hamano wrote:

Sorry, I'm still trying to understand your comment. Okay, yes: branch.$name.remote does flip explicit_default_remote_name, because we need to know if the default remote name was explicitly given. Wait, how is explicit_default_remote_name used to set default_remote_name? Don't you mean name_given? It controls name_give, yes. At this point I don't have .pushremote, yes: I'm setting up for [5/6] and [6/6]. My [6/6] doesn't seem to do the "same"? The same thing as .remote? Are you asking why .pushremote doesn't flip explicit_default_remote_name like .remote does? Because .pushremote can only ever be specified explicitly: otherwise, it falls back to the .remote logic.

Okay, next paragraph. Why isn't it necessary to add explicit_default_pushremote_name? Like I said, .pushremote can only ever be specified explicitly. There is no implicit fallback (like "origin"): it just falls back to the .remote codepath, if not explicitly specified. In other words, it's just a small override on the .remote codepath.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

That one sentence is enough to explain the apparent asymmetry, which bothered me.

This new configuration variable defines the default remote to push to, and overrides `branch.<name>.remote` for all branches. It is useful in the typical triangular-workflow setup, where the remote you're fetching from is different from the remote you're pushing to.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt | 13 ++++++++++---
 remote.c                 |  4 ++++
 t/t5516-fetch-push.sh    | 12 ++++++++++++
 3 files changed, 26 insertions(+), 3 deletions(-)

Ramkumar Ramachandra wrote:

Micronit: I think this would be easier to explain if it came after
patch 6, since then you could say "In other words, it is a default for
branch.<name>.pushremote for all branches" and readers would not have
to wonder "Why does the more generic configuration override the more
specific one?".

My two cents, Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

Thanks for all good comments (not only to this one but to others in the series). I agree with all of them.

Jonathan Nieder wrote:

I'm sorry, but this is not worth a rebase.

This new configuration variable overrides `remote.pushdefault` and `branch.<name>.remote` for pushes. In a typical triangular-workflow setup, you would want to set `remote.pushdefault` to specify the remote to push to for all branches, and use this option to override it for a specific branch.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt | 18 ++++++++++++++----
 remote.c                 |  3 +++
 t/t5516-fetch-push.sh    | 15 +++++++++++++++
 3 files changed, 32 insertions(+), 4 deletions(-)

On Wed, Mar 20, 2013 at 8:45 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Shouldn't this patch be squashed into 5/6 because of...

...this? (Since this description was introduced in 5/6)

Tay Ray Chuan wrote:

Huh? This patch introduces branch.<name>.pushremote: the relevant code and documentation changes. 5/6 introduces remote.pushdefault, which is completely different.

On Wed, Mar 20, 2013 at 8:44 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Perhaps we should clarify how this differs from remote.pushurl in the documentation for it, in git-config and/or git-push. Maybe even include the design decisions behind it from [1]. :)

http://thread.gmane.org/gmane.comp.version-control.git/215702/focus=215717

-- Cheers, Ray Chuan

Tay Ray Chuan wrote:

Actually, it's quite obvious when you think about it: remote.pushurl doesn't allow remote tracking branches, and this is a very serious limitation. If anything, the documentation for remote.pushurl should be updated to explain that it has a very narrow usecase (will do in a future patch).

From: "Ramkumar Ramachandra" <artagnon@gmail.com>
Sent: Wednesday, March 20, 2013 12:44 PM

Shouldn't Documentation/gitworkflows.txt also be updated with the triangular workflow and its configuration?

Philip Oakley wrote:

Currently, Documentation/gitworkflows.txt makes no effort to explain how to set up configuration variables for centralized or distributed workflows. I don't see how I could patch the existing document to include this workflow, without changing the entire structure of the document (left as an exercise for future patches).

"Philip Oakley" <philipoakley@iee.org> writes:

What is missing from gitworkflows documentation is actually a non-triangular workflow, where people pull from and push into the same central repository.

The "merge workflow" part in the "distributed workflows" section teaches:

 * fetching others' work from <remote> and merging it to yours;
 * publishing your work to <remote>; and
 * advertising your work.

and it is written for the triangular workflow.

Two triangles are involved. The maintainer may pull from you but pushes to her own, which is one triangle, and you pull from the maintainer and push to your own, which is another.

As to your suggestion, I do think it is reasonable to clarify these triangles with an illustration, and to even add descriptions for short-cut configurations as a side note.

Junio C Hamano wrote:

I think the gitworkflows document should be rewritten with focus on
setting up remotes and configuration variables for specific workflows.
 Once these are set up, pushing/ pulling (git pull is currently
broken, although I'm working on fixing it) should Just Work.  To push
to a different remote/ refspec, the user can read the manpage of git
push/ git pull.  A workflow is about setting up good defaults that
work 90% of the time with no additional effort.