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

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.

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

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:

What do people think?

Cheers, Wincent

On  0, Wincent Colaiuta <win@wincent.com> wrote:

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,

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

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

Wincent Colaiuta <win@wincent.com> writes:

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?

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

True, although an alias would fix that.

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.

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

On Samstag, 21. März 2009, Junio C Hamano wrote:

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(-)

On Sat, Mar 21, 2009 at 12:58:33AM -0700, Junio C Hamano wrote: [...]

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

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

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'.

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

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.

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

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.

On Tue, Mar 24, 2009 at 02:13:18AM -0700, Junio C Hamano wrote:

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

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

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...