What's cooking in git.git (Mar 2009, #06; Sat, 21)

What's cooking in git.git (Mar 2009, #06; Sat, 21)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with '-' are
only in 'pu' while commits prefixed with '+' are in 'next'.  The ones
marked with '.' do not appear in any of the branches, but I am still
holding onto them.

The topics list the commits in reverse chronological order.  The topics
meant to be merged to the maintenance series have "maint-" in their names.

----------------------------------------------------------------
[New Topics]

* jk/reflog-date (Fri Mar 20 02:00:43 2009 -0400) 1 commit
 - make oneline reflog dates more consistent with multiline format

* js/maint-1.6.0-exec-path-env (Wed Mar 18 08:42:53 2009 +0100) 1 commit
 - export GIT_EXEC_PATH when git is run with --exec-path

* da/difftool (Thu Mar 19 01:25:25 2009 -0700) 1 commit
 - difftool: move 'git-difftool' out of contrib

* jc/maint-1.6.0-keep-pack (Thu Mar 19 22:47:54 2009 -0500) 4 commits
 - Remove --kept-pack-only option and associated infrastructure
 - pack-objects: only repack or loosen objects residing in "local"
   packs
 - git-repack.sh: don't use --kept-pack-only option to pack-objects
 - t7700-repack: add two new tests demonstrating repacking flaws

Brandon Casey fixed the regression previous patches introduced; thanks.

* jc/maint-1.6.0-blame-s (Wed Mar 18 00:13:03 2009 -0700) 1 commit
 - blame: read custom grafts given by -S before calling
   setup_revisions()

The above are all ready for 'next'.

* hv/cvsps-tests (Wed Mar 18 18:33:41 2009 +0100) 7 commits
 - cvsimport: extend testcase about patchset order to contain
   branches
 - cvsimport: add test illustrating a bug in cvsps
 - Add a test of "git cvsimport"'s handling of tags and branches
 - Add some tests of git-cvsimport's handling of vendor branches
 - Test contents of entire cvsimported "master" tree contents
 - Use CVS's -f option if available (ignore user's ~/.cvsrc file)
 - Start a library for cvsimport-related tests

Two cvsimport test topics were rewound from 'next' and merged into this
one.  I'll keep this in 'pu' so that people can polish their cvsps skilz
to resolve issues these tests identify.

----------------------------------------------------------------
[Graduated to "master"]

* fc/parseopt-config (Tue Mar 17 10:46:37 2009 +0100) 10 commits
 + config: test for --replace-all with one argument and fix
   documentation.
 + config: set help text for --bool-or-int
 + git config: don't allow --get-color* and variable type
 + git config: don't allow extra arguments for -e or -l.
 + git config: don't allow multiple variable types
 + git config: don't allow multiple config file locations
 + git config: reorganize to use parseopt
 + git config: reorganize get_color*
 + git config: trivial rename in preparation for parseopt
 + git_config(): not having a per-repo config file is not an error

* js/rebase-i-opt (Tue Mar 3 10:55:31 2009 +0100) 1 commit
 + rebase -i: avoid 'git reset' when possible

* jc/clone-branch-rebase (Tue Mar 10 01:20:42 2009 -0700) 2 commits
 + Improve "git branch --tracking" output
 + Make git-clone respect branch.autosetuprebase

This is a rewrite of a patch from Pat Notz.

* xx/db-refspec-vs-js-remote (Sun Mar 8 00:12:33 2009 -0800) 1 commit
 + Adjust js/remote-improvements and db/refspec-wildcard-in-the-
   middle

* db/refspec-wildcard-in-the-middle (Sat Mar 7 01:11:39 2009 -0500) 5 commits
 + Support '*' in the middle of a refspec
 + Keep '*' in pattern refspecs
 + Use the matching function to generate the match results
 + Use a single function to match names against patterns
 + Make clone parse the default refspec with the normal code

* bw/autoconf (Thu Mar 12 15:20:12 2009 -0400) 7 commits
 + configure: rework pthread handling to allow for user defined flags
 + configure: make iconv tests aware of user arguments
 + configure: asciidoc version test cleanup
 + configure: wrap some library tests with GIT_STASH_FLAGS
 + configure: add macros to stash FLAG variables
 + configure: reorganize flow of argument checks
 + configure: ensure settings from user are also usable in the script

* mv/parseopt-ls-files (Sat Mar 7 20:27:22 2009 -0500) 4 commits
 + ls-files: fix broken --no-empty-directory
 + t3000: use test_cmp instead of diff
 + parse-opt: migrate builtin-ls-files.
 + Turn the flags in struct dir_struct into a single variable

The tip one was a subject for further discussion, but nothing is queued
yet.

----------------------------------------------------------------
[Will merge to 'master' soon]

* dm/maint-docco (Thu Mar 19 20:35:34 2009 -0700) 6 commits
 + Documentation: reword example text in git-bisect.txt.
 + Documentation: reworded the "Description" section of git-
   bisect.txt.
 + Documentation: minor grammatical fixes in git-branch.txt.
 + Documentation: minor grammatical fixes in git-blame.txt.
 + Documentation: reword the "Description" section of git-bisect.txt.
 + Documentation: minor grammatical fixes in git-archive.txt.

* mg/test-installed (Mon Mar 16 18:03:12 2009 +0100) 2 commits
 + test-lib.sh: Allow running the test suite against installed git
 + test-lib.sh: Test for presence of git-init in the right path.

----------------------------------------------------------------
[Discarded]

* hv/cvsimport-tests (Mon Mar 2 18:59:36 2009 +0100) 1 commit
 ? cvsimport: add test illustrating a bug in cvsps

* mh/cvsimport-tests (Mon Feb 23 06:08:14 2009 +0100) 5 commits
 ? Add a test of "git cvsimport"'s handling of tags and branches
 ? Add some tests of git-cvsimport's handling of vendor branches
 ? Test contents of entire cvsimported "master" tree contents
 ? Use CVS's -f option if available (ignore user's ~/.cvsrc file)
 ? Start a library for cvsimport-related tests

----------------------------------------------------------------
[Stalled and may need help and prodding to go forward]

* ps/blame (Thu Mar 12 21:30:03 2009 +1100) 1 commit
 - blame.c: start libifying the blame infrastructure

A few minor point remains in this initial one.

* jc/log-tz (Tue Mar 3 00:45:37 2009 -0800) 1 commit
 - Allow --date=local --date=other-format to work as expected

The one I posted had a few corner-case bugs that was caught with the test
suite; this one has them fixed.  People did not like the UI so it is kept
out of 'next'

* lh/submodule-tree-traversal (Sun Jan 25 01:52:06 2009 +0100) 1 commit
 - archive.c: add support for --submodules[=(all|checkedout)]

Discussion stalled on the submodule selection criteria.
Probably I should discard it and wait for a reroll if needed.

* jc/merge-convert (Mon Jan 26 16:45:01 2009 -0800) 1 commit
 - git-merge-file: allow converting the results for the work tree

This is a feature waiting for a user.

We did not give scripted Porcelains a way to say "this temporary file I am
using for merging is for this path, so use the core.autocrlf and attributes
rules for that final path".  Instead, merge-file simply wrote out the
data in the canonical repository representation.

rerere has the same issue, but it is a lot worse.  It reads the three
files (preimage, postimage and thisimage) from the work tree in the work
tree representation, merges them without converting them to the canonical
representation first but inserts the conflict markers with the canonical
representation and writes the resulting mess out.  It needs to be fixed to
read with convert_to_git(), merge them while they are still in the
canonical representation and possibly add conflict markers, and then write
the results out after convert_to_working_tree().  It also needs to write
in binary mode as well.

* db/foreign-scm (Sun Jan 11 15:12:10 2009 -0500) 3 commits
 - Support fetching from foreign VCSes
 - Add specification of git-vcs helpers
 - Add "vcs" config option in remotes

Daniel seems to have a plan for going forward with this series.

* cc/replace (Mon Feb 2 06:13:06 2009 +0100) 11 commits
 - builtin-replace: use "usage_msg_opt" to give better error messages
 - parse-options: add new function "usage_msg_opt"
 - builtin-replace: teach "git replace" to actually replace
 - Add new "git replace" command
 - environment: add global variable to disable replacement
 - mktag: call "check_sha1_signature" with the replacement sha1
 - replace_object: add a test case
 - object: call "check_sha1_signature" with the replacement sha1
 - sha1_file: add a "read_sha1_file_repl" function
 - replace_object: add mechanism to replace objects found in
   "refs/replace/"
 - refs: add a "for_each_replace_ref" function

I know, I really have to drop everything else and re-read these, but I
haven't managed to.

* js/notes (Wed Feb 18 11:17:27 2009 -0800) 14 commits
 - tests: fix "export var=val"
 - notes: refuse to edit notes outside refs/notes/
 - t3301: use test_must_fail instead of !
 - t3301: fix confusing quoting in test for valid notes ref
 - notes: use GIT_EDITOR and core.editor over VISUAL/EDITOR
 - notes: only clean up message file when editing
 - handle empty notes gracefully
 - git notes show: test empty notes
 - git-notes: fix printing of multi-line notes
 - notes: fix core.notesRef documentation
 - Add an expensive test for git-notes
 - Speed up git notes lookup
 - Add a script to edit/inspect notes
 - Introduce commit notes

----------------------------------------------------------------
[Actively cooking]

* jc/attributes-checkout (Fri Mar 20 10:32:09 2009 +0100) 2 commits
 - Add a test for checking whether gitattributes is honored by
   checkout.
 - Read attributes from the index that is being checked out

Original issue identified, and test provided by Kristian Amlie.

* fg/push-default (Mon Mar 16 16:42:52 2009 +0100) 2 commits
 - Display warning for default git push with no push.default config
 + New config push.default to decide default behavior for push

Replaced the old series with the first step to allow a smooth transition.
Some might argue that this should not give any warning but just give users
this new configuration to play with first, and after we know we are going
to switch default some day, start the warning.

* mg/http-auth (Wed Mar 18 18:46:41 2009 -0500) 6 commits
 + http-push.c: use a faux remote to pass to http_init
 + Do not name "repo" struct "remote" in push_http.c
 + http.c: CURLOPT_NETRC_OPTIONAL is not available in ancient
   versions of cURL
 + http authentication via prompts
 + http_init(): Fix config file parsing
 + http.c: style cleanups

Amos King added push side support on top of my fetch side support.

We may want to also pass --remote parameter from git-push to this backend
as Daniel did as an interim solution for the fetch side, so that we can
handle the configuration better.

* db/push-cleanup (Sun Mar 8 21:06:07 2009 -0400) 2 commits
 + Move push matching and reporting logic into transport.c
 + Use a common function to get the pretty name of refs

* kb/tracking-count-no-merges (Wed Mar 4 18:47:39 2009 +0100) 1 commit
 + stat_tracking_info(): only count real commits

This gives the merge commits zero weight when talking about how many
commits you have ahead (or behind) of the branch you are tracking.  Even
though I agree that they should carry much less weight than the "real"
commits, because your repeated merge from the other branch does not really
add any real value to the end result, giving them absolute zero weight
somehow feels wrong. At least it shows that your have been _active_ on the
branch.  But I do not feel very strongly about it.

----------------------------------------------------------------
[On Hold]

* jc/deny-delete-current-1.7.0 (Mon Feb 9 00:19:46 2009 -0800) 1 commit
 - receive-pack: default receive.denyDeleteCurrent to refuse

* jc/refuse-push-to-current-1.7.0 (Wed Feb 11 02:28:03 2009 -0800) 1 commit
 - Refuse updating the current branch in a non-bare repository via
   push

These are for 1.7.0, but the messages when they trigger together may need
to be rethought.

Re: What's cooking in git.git (Mar 2009, #06; Sat, 21)

[Wincent Colaiuta]

El 21/3/2009, a las 8:58, Junio C Hamano escribió:

> * da/difftool (Thu Mar 19 01:25:25 2009 -0700) 1 commit
> - difftool: move 'git-difftool' out of contrib

Before this one goes any further, I noticed that nobody replied to my  
email on this thread a few days ago:

http://article.gmane.org/gmane.comp.version-control.git/113609

My concern was:

> Given that git-difftool shares basically all the same options as  
> "git diff", I think a good long-term plan would be looking at adding  
> the "--tool" option to "git diff" itself so that users wouldn't have  
> to learn a new subcommand, just a new option.


What do people think?

Cheers,
Wincent

Re: What's cooking in git.git (Mar 2009, #06; Sat, 21)

[David Aguilar]

On  0, Wincent Colaiuta <win@wincent.com> wrote:
> El 21/3/2009, a las 8:58, Junio C Hamano escribió:
>
>> * da/difftool (Thu Mar 19 01:25:25 2009 -0700) 1 commit
>> - difftool: move 'git-difftool' out of contrib
>
> Before this one goes any further, I noticed that nobody replied to my  
> email on this thread a few days ago:
>
> http://article.gmane.org/gmane.comp.version-control.git/113609
>
> My concern was:
>
>> Given that git-difftool shares basically all the same options as "git 
>> diff", I think a good long-term plan would be looking at adding the 
>> "--tool" option to "git diff" itself so that users wouldn't have to 
>> learn a new subcommand, just a new option.
>
>
> What do people think?
>
> Cheers,
> Wincent

That could be interesting.  git-difftool is just a
frontend to git-diff so there isn't really any maintenance
worries about keeping the options in sync.  I do agree that
keeping things easy for users is a noble goal and that that
was your only concern.

git-difftool is pure porcelain, so I'm interested in how we
could implement this.  Right now the call stack is:

$ git difftool
... GIT_EXT_DIFF=git-difftool-helper
... git diff
... ... git-difftool-helper
... ... ... xxdiff


What should it look like instead?

Are you envisioning this (1):

$ git diff --tool
... --tool was passed, so set GIT_EXT_DIFF?
... git-difftool-helper
... ... xxdiff ...


Or do you mean this? (2):

$ git diff --tool=xxdiff
... --tool was passed, so set GIT_EXT_DIFF?
... git-difftool-helper
... ... xxdiff


Or even... (3):

$ git diff --tool
... --tool was passed, delegate to git-diff--tool,
...        remove --tool from *argv
... git-diff--tool
... GIT_EXT_DIFF=git-diff--tool-helper
... git-diff
... ... git-diff--tool-helper
... ... ... xxdiff ...

(git-diff calls itself in this scenario...)


Right now users only specify --tool=<tool> as an override.

The default behavior without --tool is:
- difftool looks up diff.tool and uses that value, or
- difftool makes a best guess based on the environ

Hmm.. if --tool supported both '--tool' and '--tool=<tool>'
then that could work.  That would make '--tool' both a switch
and an option-with-argument.  Is there anything in git that
does that?  I can imaging that being a little surprising from
a ui point of view, but it's not horrible.

What about the --no-prompt option?
Would we need that in git-diff too, or would we be able to
blindly pass it along to git-diff--tool without worrying that
git-diff would try to interpret it?

I personally like the separation of concerns --
git-diff is plumbing and git-difftool is porcelain.
But, I also agree with making it easier for users.


That said...  (off-topic)

I have a patch for difftool that lets you do stuff like this:

	$ git difftool Makefile~3
	$ git difftool Makefile~~~ Makefile~

That is *completely* the antithesis of git because git is not
file-centric.  Nonetheless, this is something people ask me
all the time and users really hate the "right" way to do it:

	$ git difftool \
		$(git log --pretty=format:%H -3 Makefile |
			tail -1) \
		-- Makefile


The point here is that since git-difftool is a frontend to
git-diff I was actually able to implement it without changing
any of the core git commands (or extend its revision syntax).
This is both good and bad.  It's good because users are much
happier using the extended file-rev syntax, and it's bad
because git-diff doesn't know about it.

What it illustrates, though, is that the separation of
concerns between the porcelain git-difftool and plumbing
git-diff is helpful specifically because it makes such things
possible.

The really cute thing, though, is that
	$ git difftool --no-ext-diff Makefile~3

...actually makes it so that git-diff understands the new
syntax.  It's quite a clever hack.  It's user-friendly
and extremely helpful, which is why I entertained the
notion of implementing it.  It basically transforms user intent
"give me Makefile from 3 changes ago back" into something that
git-diff understands, which is in my opinion the whole point of
porcelains.

That said.. it would be really user-friendly if diff and
friends understood the extended file-rev syntax directly, but
being that it overloads the '~' character and "looks" just
like a rev-parse revision specifier, I don't even know if its:

	a. possible, or
	b. something we'd want given that git is
	   commit-centric, not file-centric.

I should really start a second thread about the file-rev
syntax because I made it out of thin air and it's exactly the
kind of thing that the wisdom of the list can help vet.


An interesting thing is that users have been mailing me
directly with questions about difftool and I really want to
use the full potential of the community, which I think will
come naturally with the move out of contrib.

So.. I agree in principle but also think it wouldn't hurt to
go forward with moving git-difftool out of contrib so that we
can get more user feedback.  I also think that a scheme most
similar to (3) above seems like an interesting way to go and
would be interested to hear if it's what you envisioned.


I hope I didn't just muddy the waters further =)

Have fun,

-- 

	David

Re: What's cooking in git.git (Mar 2009, #06; Sat, 21)

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

>> Given that git-difftool shares basically all the same options as
>> "git diff", I think a good long-term plan would be looking at adding
>> the "--tool" option to "git diff" itself so that users wouldn't have
>> to learn a new subcommand, just a new option.
>
> What do people think?

I am not "people" but since I was "To:"ed not "Cc:"ed...

I did not comment on it because personally I was not very interested in
it; admittedly, I do not use difftool myself, but:

 (1) "git diff --tool" is more to type than "git difftool"; and

 (2) it requires adding more code to "git diff" for a dubious benefit from
     end user's point of view.

"git diff" itself is already just a thin command dispatcher that calls
underlying vastly different modes operations, and it is designed that way
in order to make it easy to add a new totally different mode of operation.

We originally had four underlying modes of operation (index-with-worktree,
tree-with-tree, tree-with-worktree, and tree-with-index), and Dscho
managed to add random-file-with-random-file without disturbing existing
codepaths too much.

Even though it is a "thin" dispatcher, command line parsing is largely
done by "git diff" itself, and its results are fed to the underlying modes
of operations, for two reasons:

 (1) These "modes of operations" share large set of options (e.g. "-U20"
     works the same way no matter which mode you invoke); and

 (2) the dispatcher needs to inspect command line option and arguments to
     decide which mode to invoke anyway.

When an end user says "I want to compare two things with these settings
(e.g. use color, with 5 lines of context, only inside Documentation/howto
directory, detect renames with lower-than-usual threashold, ...)", the
mental model is same whether the two things being compared happens to be
index-vs-worktree or tree-vs-index from the end user's point of view.  It
makes a lot of sense for "git diff --options" to invoke both modes of
operations with a similar-looking command line.

Even though the --no-index mode of operation internally does not fit very
well compared to the original four modes from the implementation point of
view, it still naturally fits the end user's mental model and that is why
it is given as an option to "git diff".

Does "git difftool" fit well in the end user's mental model in a similar
way to "git diff"?  I somehow suspect it doesn't.  What does it mean to
give "-U8 --color --stat --summary -p --ignore-space-at-eol" options when
you invoke it, for example?

Re: What's cooking in git.git (Mar 2009, #06; Sat, 21)

[Johannes Sixt]

On Samstag, 21. März 2009, Junio C Hamano wrote:
> * js/maint-1.6.0-exec-path-env (Wed Mar 18 08:42:53 2009 +0100) 1 commit
>  - export GIT_EXEC_PATH when git is run with --exec-path

Here is the patch again with a commit message that explains what's going on.

But notice that this is probably not a complete solution: As you can see
(below) from the analysis, a git-repack that was taken from $prefix would
invoke a git that is not from $prefix, but from the initial command's
--exec-path. I think this can be solved by installing the git executable
in $prefix/libexec/git-core as well.

-- Hannes

-- 8< --
Subject: [PATCH] Propagate --exec-path setting to external commands via GIT_EXEC_PATH

Let PATH0=$PATH that was set before the invocation.
Let /foo be a build directory.
Let /pfx be the installation prefix.
Let pfxexecpath=/pfx/libexec/git-core.

The following is going on when 'git --exec-path=/foo gc' is invoked:

1. git sets PATH=/foo:$PATH0 using the path from --exec-path

2. gc execs 'git repack' (note: no dash).

3. Since there is a git in /foo (it's a build directory), /foo/git is
   taken.

4. No explicit exec-path is set this time, hence, this secondary git sets
   PATH=$pfxexecpath:/foo:$PATH

5. Since 'repack' is not a built-in, execv_dashed_external execs
   'git-repack' (note: dash).

6. There is a $pfxexecpath/git-repack, and it is taken.

7. This git-repack runs 'git pack-objects' (note: no dash).

8. There is no git in $pfxexecpath, but there is one in /foo. Hence,
   /foo/git is run.

9. pack-objects is a builtin, hence, in effect /foo/git-pack-objects
   is run.

As you can see, the way in which we previously set the PATH allowed to
mix gits of different vintage.  By setting GIT_EXEC_PATH when --exec-path
was given on the command line, we reduce the confusion.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 exec_cmd.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 217c125..408e4e5 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -61,6 +61,10 @@ const char *git_extract_argv0_path(const char *argv0)
 void git_set_argv_exec_path(const char *exec_path)
 {
 	argv_exec_path = exec_path;
+	/*
+	 * Propagate this setting to external programs.
+	 */
+	setenv(EXEC_PATH_ENVIRONMENT, exec_path, 1);
 }
 
 
-- 
1.6.2.1.224.g2225f

Re: What's cooking in git.git (Mar 2009, #06; Sat, 21)

[Wincent Colaiuta]

El 21/3/2009, a las 20:28, Junio C Hamano escribió:

> Wincent Colaiuta <win@wincent.com> writes:
>
>>> Given that git-difftool shares basically all the same options as
>>> "git diff", I think a good long-term plan would be looking at adding
>>> the "--tool" option to "git diff" itself so that users wouldn't have
>>> to learn a new subcommand, just a new option.
>>
>> What do people think?
>
> I am not "people" but since I was "To:"ed not "Cc:"ed...
>
> I did not comment on it because personally I was not very interested  
> in
> it; admittedly, I do not use difftool myself, but:
>
> (1) "git diff --tool" is more to type than "git difftool"; and

True, although an alias would fix that.

> (2) it requires adding more code to "git diff" for a dubious benefit  
> from
>     end user's point of view.

Fair enough. I was just wondering if we could avoid adding yet another  
command to the already long list of installed commands. I understand  
that "diff" is actually a thin dispatcher for different modes of  
operation, but seeing as all of those modes basically boil down to  
"show me the difference between two things", and that happens to be  
the purpose of "difftool" as well, I thought it might make sense to  
combine them.

> When an end user says "I want to compare two things with these  
> settings
> (e.g. use color, with 5 lines of context, only inside Documentation/ 
> howto
> directory, detect renames with lower-than-usual threashold, ...)", the
> mental model is same whether the two things being compared happens  
> to be
> index-vs-worktree or tree-vs-index from the end user's point of  
> view.  It
> makes a lot of sense for "git diff --options" to invoke both modes of
> operations with a similar-looking command line.
>
> Even though the --no-index mode of operation internally does not fit  
> very
> well compared to the original four modes from the implementation  
> point of
> view, it still naturally fits the end user's mental model and that  
> is why
> it is given as an option to "git diff".
>
> Does "git difftool" fit well in the end user's mental model in a  
> similar
> way to "git diff"?  I somehow suspect it doesn't.  What does it mean  
> to
> give "-U8 --color --stat --summary -p --ignore-space-at-eol" options  
> when
> you invoke it, for example?

Good question. Seeing as right now "difftool" just passes all options  
through to "diff", I guess it's up to the user to pass in options  
which actually make sense.

Cheers,
Wincent

Re: What's cooking in git.git (Mar 2009, #06; Sat, 21)

[Wincent Colaiuta]

El 21/3/2009, a las 19:58, David Aguilar escribió:

> On  0, Wincent Colaiuta <win@wincent.com> wrote:
>> El 21/3/2009, a las 8:58, Junio C Hamano escribió:
>>
>>> * da/difftool (Thu Mar 19 01:25:25 2009 -0700) 1 commit
>>> - difftool: move 'git-difftool' out of contrib
>>
>> Before this one goes any further, I noticed that nobody replied to my
>> email on this thread a few days ago:
>>
>> http://article.gmane.org/gmane.comp.version-control.git/113609
>>
>> My concern was:
>>
>>> Given that git-difftool shares basically all the same options as  
>>> "git
>>> diff", I think a good long-term plan would be looking at adding the
>>> "--tool" option to "git diff" itself so that users wouldn't have to
>>> learn a new subcommand, just a new option.
>>
>>
>> What do people think?
>>
>> Cheers,
>> Wincent
>
> That could be interesting.  git-difftool is just a
> frontend to git-diff so there isn't really any maintenance
> worries about keeping the options in sync.  I do agree that
> keeping things easy for users is a noble goal and that that
> was your only concern.
>
> git-difftool is pure porcelain, so I'm interested in how we
> could implement this.  Right now the call stack is:
>
> $ git difftool
> ... GIT_EXT_DIFF=git-difftool-helper
> ... git diff
> ... ... git-difftool-helper
> ... ... ... xxdiff
>
>
> What should it look like instead?
>
> Are you envisioning this (1):
>
> $ git diff --tool
> ... --tool was passed, so set GIT_EXT_DIFF?
> ... git-difftool-helper
> ... ... xxdiff ...

Yeah, something like that. I wasn't actually imagining that the logic  
of git-difftool-helper would itself be rolled into "git diff". I was  
just wondering if we could keep the command-count down.

Cheers,
Wincent

Re: What's cooking in git.git (Mar 2009, #06; Sat, 21)

[Finn Arne Gangstad]

On Sat, Mar 21, 2009 at 12:58:33AM -0700, Junio C Hamano wrote:
[...]
> * fg/push-default (Mon Mar 16 16:42:52 2009 +0100) 2 commits
>  - Display warning for default git push with no push.default config
>  + New config push.default to decide default behavior for push
> 
> Replaced the old series with the first step to allow a smooth transition.
> Some might argue that this should not give any warning but just give users
> this new configuration to play with first, and after we know we are going
> to switch default some day, start the warning.

If you feel that talking about a possible future change is premature,
you could omit that part of the second commit I guess, but I think
printing some kind of warning is valuable.  Are you waiting for more
input?  It seems that this topic is pretty dead now.

Most people who get bitten by this directly are probably not active on
this list so I don't think you will hear from many of them.

- Finn Arne

Re: What's cooking in git.git (Mar 2009, #06; Sat, 21)

[Junio C Hamano]

Finn Arne Gangstad <finnag@pvv.org> writes:

> On Sat, Mar 21, 2009 at 12:58:33AM -0700, Junio C Hamano wrote:
> [...]
>> * fg/push-default (Mon Mar 16 16:42:52 2009 +0100) 2 commits
>>  - Display warning for default git push with no push.default config
>>  + New config push.default to decide default behavior for push
>> 
>> Replaced the old series with the first step to allow a smooth transition.
>> Some might argue that this should not give any warning but just give users
>> this new configuration to play with first, and after we know we are going
>> to switch default some day, start the warning.
>
> If you feel that talking about a possible future change is premature,
> you could omit that part of the second commit I guess, but I think
> printing some kind of warning is valuable.  Are you waiting for more
> input?  It seems that this topic is pretty dead now.
>
> Most people who get bitten by this directly are probably not active on
> this list so I don't think you will hear from many of them.

I have already judged that the cause of this series is good, and that is
why the series was even considered to be in git.git to begin with.  I also
looked at the code change in them, and I found it Ok, and that is why the
first one is queued in 'next'.

At this point we do not need "Yes, it is a good idea" from people, even
though "No, it is a horrible change because of such and such reasons"
could reverse its course, if the argument is new.

The reason the patch has been sitting in 'next' is entirely unrelated to
the above.  It is to hear from people about unintended side effects, if
any.  It's only been a week, isn't it?

Unlike documentation reformatting and other kinds of patches in which
potential breakages will not cause a disaster, push is one of the things
we would want to keep working for people.  In general, the more important
the area a patch touches, the patch needs to cook longer in 'next'.

Re: What's cooking in git.git (Mar 2009, #06; Sat, 21)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Finn Arne Gangstad <finnag@pvv.org> writes:
>
>> If you feel that talking about a possible future change is premature,
>> you could omit that part of the second commit I guess, but I think
>> printing some kind of warning is valuable.  Are you waiting for more
>> input?  It seems that this topic is pretty dead now.

Now it turns out that it has been "pretty dead" because nobody, not even
the original author, was doing any testing and fixing.

After merging this "warning" thing to next, I mistype the name of the
remote I wanted to push with the default "matching" semantics and got this:

  $ git push --dry-run sf.net
  warning: You did not specify any refspecs to push, and the current remote
  warning: has not configured any push refspecs. The default action in this
  warning: case is to push all matching refspecs, that is, all branches
  warning: that exist both locally and remotely will be updated.  This may
  warning: not necessarily be what you want to happen.
  warning:
  warning: You can specify what action you want to take in this case, and
  warning: avoid seeing this message again, by configuring 'push.default' to:
  warning:   'nothing'  : Do not push anythig
  warning:   'matching' : Push all matching branches (default)
  warning:   'tracking' : Push the current branch to whatever it is tracking
  warning:   'current'  : Push the current branch
  fatal: 'sf.net' does not appear to be a git repository
  fatal: The remote end hung up unexpectedly

The final, most important error messages are dwarfed out by the warning
that talks about setting configuration on the remote that does not even
exist.

In this particular case, it does not corrupt the local nor remote
repositories, and because it was me who tried this who knew what he was
doing, so it is Ok, and that is the point of keeping any new features out
of 'master' until such silly misfeatures are found and fixed.  But it
would have been nicer if I weren't the only one testing and finding bugs.

Re: What's cooking in git.git (Mar 2009, #06; Sat, 21)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>   $ git push --dry-run sf.net
>   warning: You did not specify any refspecs to push, and the current remote
>   warning: has not configured any push refspecs. The default action in this
>   warning: case is to push all matching refspecs, that is, all branches
>   warning: that exist both locally and remotely will be updated.  This may
>   warning: not necessarily be what you want to happen.
>   warning:
>   warning: You can specify what action you want to take in this case, and
>   warning: avoid seeing this message again, by configuring 'push.default' to:
>   warning:   'nothing'  : Do not push anythig
>   warning:   'matching' : Push all matching branches (default)
>   warning:   'tracking' : Push the current branch to whatever it is tracking
>   warning:   'current'  : Push the current branch
>   fatal: 'sf.net' does not appear to be a git repository
>   fatal: The remote end hung up unexpectedly
>
> The final, most important error messages are dwarfed out by the warning
> that talks about setting configuration on the remote that does not even
> exist.

Actually, I take it back.  It is still annoying, but the point of these
warning lines is to warn even for a one-off push you make to a place
without having any [remote "sf.net"] entry anywhere in the config.  In the
worst case, the above "sf.net" may even be just a full URL of the remote,
and we do want to trigger the warning.

So this is not even a usability bug.  Sorry for a thinko.

Re: What's cooking in git.git (Mar 2009, #06; Sat, 21)

[Finn Arne Gangstad]

On Tue, Mar 24, 2009 at 02:13:18AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >   $ git push --dry-run sf.net
> >   warning: You did not specify any refspecs to push, and the current remote
> >   warning: has not configured any push refspecs. The default action in this
> >   warning: case is to push all matching refspecs, that is, all branches
> >   warning: that exist both locally and remotely will be updated.  This may
> >   warning: not necessarily be what you want to happen.
> >   warning:
> >   warning: You can specify what action you want to take in this case, and
> >   warning: avoid seeing this message again, by configuring 'push.default' to:
> >   warning:   'nothing'  : Do not push anythig
> >   warning:   'matching' : Push all matching branches (default)
> >   warning:   'tracking' : Push the current branch to whatever it is tracking
> >   warning:   'current'  : Push the current branch
> >   fatal: 'sf.net' does not appear to be a git repository
> >   fatal: The remote end hung up unexpectedly
> >
> > The final, most important error messages are dwarfed out by the warning
> > that talks about setting configuration on the remote that does not even
> > exist.

I actually agree, but that final error is printed in a very ugly
place! It would require some surgery to pause the warning until we
figure out that sf.net is not a valid remote in the current setup.

get_refs_via_connect->connect_setup->git_connect-> .. fork()s .., runs
   "sh -c git-receive-pack 'sf.net'" (which prints the first error)
and get_remote_heads->packet_read_line->safe_read() dumps the final error
and dies.

Is there any reason why remote_get needs to return a valid remote for
a value like "sf.net"?  If it didn't, the error message would be even
better, and not complain about a "remote end".

Just thinking aloud, if what is specified as a remote does not contain
a ":" it cannot really be a URL(?), and we can assume it is a local
directory. If that directory does not exist is not a valid git
repository, it might be safe to fail in remote_get?

- Finn Arne

Re: What's cooking in git.git (Mar 2009, #06; Sat, 21)

[Junio C Hamano]

Finn Arne Gangstad <finnag@pvv.org> writes:

> On Tue, Mar 24, 2009 at 02:13:18AM -0700, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> >   $ git push --dry-run sf.net
>> >   warning: ...
>> >   warning: You can specify what action you want to take in this case, and
>> >   warning: avoid seeing this message again, by configuring 'push.default' to:
>> >...
>> >   warning:   'current'  : Push the current branch
>> >   fatal: 'sf.net' does not appear to be a git repository
>> >   fatal: The remote end hung up unexpectedly
>> >
>> > The final, most important error messages are dwarfed out by the warning
>> > that talks about setting configuration on the remote that does not even
>> > exist.
>
> I actually agree, but that final error is printed in a very ugly
> place! It would require some surgery to pause the warning until we
> figure out that sf.net is not a valid remote in the current setup.
>
> get_refs_via_connect->connect_setup->git_connect-> .. fork()s .., runs
>    "sh -c git-receive-pack 'sf.net'" (which prints the first error)
> and get_remote_heads->packet_read_line->safe_read() dumps the final error
> and dies.
>
> Is there any reason why remote_get needs to return a valid remote for
> a value like "sf.net"?  If it didn't, the error message would be even
> better, and not complain about a "remote end".

Yeah, you could work around by special casing colon-less ones as you
suggested, but I do not think it is worth it.

Instead of "sf.net", the example could have been

	git-core.git.sourceforge.net:/gitroot/gut-core

which looks like a perfectly valid push destination, but it has a typo in
the pathname, and the remote end will hang up unexpectedly in such a case,
too.  I do not think remote_get() is a wrong thing to blame; in that
codepath you simply do not know.

If you want to remove warning when we will eventually go "fatal", the way
to do so is to remember you need to issue a warning but defer it until you
actually start telling the other end in do_send_pack(), or something like
that.

I do not think we would want nor need to go that route.  This warning is
about a local configuration that applies to _all_ remotes, and setting the
configuration once will squelch it for future push to any remotes.  Giving
the warning even for an _incorrect_ remote is not wrong per-se.

Issuing the warning at first looked incorrect to me, but it warns exactly
about what the patch that added the warning messages wanted to warn about:

    If you did not misspell the name of the remote or the URL, you would
    have triggered the default "matching refs" semantics, but you may not
    have meant to do so, hence we are warning.  If you want to change the
    default, here is how.  If you find this message irritating, you can
    squelch it this way.

In this particular case, I _did_ want the default matching refs semantics,
so it was irritating whether I had the last two "fatal" lines or not.  But
the point of your patch was to help new people for the cost of irritating
people like me (once), so...