threads / discuss / 32872

What's cooking in git.git (Feb 2013, #04; Sat, 9)

Subject: What's cooking in git.git (Feb 2013, #04; Sat, 9)

## tl;dr

11 messages between Feb 9, 2013 and Feb 11, 2013.

replies: 10people: 3as markdown or json

Junio C Hamano· Feb 9, 2013, 23:39 UTC · lore

Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'.

As usual, this cycle is expected to last for 8 to 10 weeks, with a preview -rc0 late next week. I expect that topics that are not mentioned in this message will not have enough time to be in the upcomming release (unless they are obvious regression fixes or documentation updates).

You can find the changes described here in the integration branches of the repositories listed at

    http://git-blame.blogspot.com/p/git-public-repositories.html

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

* ab/gitweb-use-same-scheme (2013-01-28) 1 commit
  (merged to 'next' on 2013-02-02 at 7e4a108)
 + gitweb: refer to picon/gravatar images over the same scheme
 Avoid mixed contents on a page coming via http and https when
 gitweb is hosted on a https server.
* ct/autoconf-htmldir (2013-02-02) 1 commit
  (merged to 'next' on 2013-02-05 at bba4f8c)
 + Honor configure's htmldir switch
 The autoconf subsystem passed --mandir down to generated
 config.mak.autogen but forgot to do the same for --htmldir.
* da/mergetool-docs (2013-02-02) 5 commits
  (merged to 'next' on 2013-02-03 at f822dcf)
 + doc: generate a list of valid merge tools
 + mergetool--lib: list user configured tools in '--tool-help'
 + mergetool--lib: add functions for finding available tools
 + mergetool--lib: improve the help text in guess_merge_tool()
 + mergetool--lib: simplify command expressions
 (this branch uses jk/mergetool.)
 Build on top of the clean-up done by jk/mergetool and automatically
 generate the list of mergetool and difftool backends the build
 supports to be included in the documentation.
* jc/combine-diff-many-parents (2013-02-05) 2 commits
  (merged to 'next' on 2013-02-05 at e382aa6)
 + t4038: add tests for "diff --cc --raw <trees>"
 + combine-diff: lift 32-way limit of combined diff
 We used to have an arbitrary 32 limit for combined diff input,
 resulting in incorrect number of leading colons shown when showing
 the "--raw --cc" output.
* jk/apply-similaritly-parsing (2013-02-03) 1 commit
  (merged to 'next' on 2013-02-05 at ccf1c97)
 + builtin/apply: tighten (dis)similarity index parsing
 Make sure the similarity value shown in the "apply --summary"
 output is sensible, even when the input had a bogus value.
* jk/doc-makefile-cleanup (2013-02-01) 1 commit
  (merged to 'next' on 2013-02-02 at 86ff373)
 + Documentation/Makefile: clean up MAN*_TXT lists
* jk/mergetool (2013-01-28) 8 commits
  (merged to 'next' on 2013-02-03 at 2ff5dee)
 + mergetools: simplify how we handle "vim" and "defaults"
 + mergetool--lib: don't call "exit" in setup_tool
 + mergetool--lib: improve show_tool_help() output
 + mergetools/vim: remove redundant diff command
 + git-difftool: use git-mergetool--lib for "--tool-help"
 + git-mergetool: don't hardcode 'mergetool' in show_tool_help
 + git-mergetool: remove redundant assignment
 + git-mergetool: move show_tool_help to mergetool--lib
 (this branch is used by da/mergetool-docs.)
 Cleans up mergetool/difftool combo.
* jk/python-styles (2013-01-30) 1 commit
  (merged to 'next' on 2013-02-02 at 293edc1)
 + CodingGuidelines: add Python coding guidelines
* jk/remote-helpers-doc (2013-02-01) 1 commit
  (merged to 'next' on 2013-02-02 at ce1461a)
 + Rename {git- => git}remote-helpers.txt
 "git help remote-helpers" did not work; 'remote-helpers' is not
 a subcommand name but a concept, so its documentation should have
 been in gitremote-helpers, not git-remote-helpers.
* mk/tcsh-complete-only-known-paths (2013-02-03) 1 commit
  (merged to 'next' on 2013-02-05 at 4409b08)
 + completion: handle path completion and colon for tcsh script
 (this branch uses mp/complete-paths.)
 Manlio's "complete with known paths only" update to completion
 scripts returns directory names without trailing slash to
 compensate the addition of '/' done by bash that reads from our
 completion result.  tcsh completion code that reads from our
 internal completion result does not add '/', so let it ask our
 complletion code to keep the '/' at the end.
* mp/complete-paths (2013-01-11) 1 commit
  (merged to 'next' on 2013-01-30 at 70e4f1a)
 + git-completion.bash: add support for path completion
 (this branch is used by mk/tcsh-complete-only-known-paths.)
 The completion script used to let the default completer to suggest
 pathnames, which gave too many irrelevant choices (e.g. "git add"
 would not want to add an unmodified path).  Teach it to use a more
 git-aware logic to enumerate only relevant ones.
* nd/branch-error-cases (2013-01-31) 6 commits
  (merged to 'next' on 2013-02-02 at cf5e745)
 + branch: let branch filters imply --list
 + docs: clarify git-branch --list behavior
 + branch: mark more strings for translation
 + Merge branch 'nd/edit-branch-desc-while-detached' into HEAD
 + branch: give a more helpful message on redundant arguments
 + branch: reject -D/-d without branch name
 Fix various error messages and conditions in "git branch", e.g. we
 advertised "branch -d/-D" to remove one or more branches but actually
 implemented removal of zero or more branches---request to remove no
 branches was not rejected.
* sb/gpg-i18n (2013-01-31) 1 commit
  (merged to 'next' on 2013-02-02 at 7a54574)
 + gpg: allow translation of more error messages
* sb/run-command-fd-error-reporting (2013-02-01) 1 commit
  (merged to 'next' on 2013-02-02 at be7e970)
 + run-command: be more informative about what failed
* ss/mergetools-tortoise (2013-02-01) 2 commits
  (merged to 'next' on 2013-02-03 at d306b83)
 + mergetools: teach tortoisemerge to handle filenames with SP correctly
 + mergetools: support TortoiseGitMerge
 Update mergetools to work better with newer merge helper tortoise ships.

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

* jc/fetch-raw-sha1 (2013-02-07) 4 commits
 - fetch: fetch objects by their exact SHA-1 object names
 - upload-pack: optionally allow fetching from the tips of hidden refs
 - fetch: use struct ref to represent refs to be fetched
 - parse_fetch_refspec(): clarify the codeflow a bit
 (this branch uses jc/hidden-refs.)
 Allows requests to fetch objects at any tip of refs (including
 hidden ones).  It seems that there may be use cases even outside
 Gerrit (e.g. $gmane/215701).
* jk/diff-graph-cleanup (2013-02-07) 6 commits
 - combine-diff.c: teach combined diffs about line prefix
 - diff.c: use diff_line_prefix() where applicable
 - diff: add diff_line_prefix function
 - diff.c: make constant string arguments const
 - diff: write prefix to the correct file
 - graph: output padding for merge subsequent parents
 Refactors a lot of repetitive code sequence from the graph drawing
 code and adds it to the combined diff output.
 Will merge to 'next'.
* mn/send-email-works-with-credential (2013-02-07) 5 commits
 - git-send-email: use git credential to obtain password
 - Git.pm: add interface for git credential command
 - Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe
 - Git.pm: fix example in command_close_bidi_pipe documentation
 - Git.pm: allow command_close_bidi_pipe to be called as method
 Hooks the credential system to send-email.
 Expecting a reroll.
 $gmane/215752
* tz/perl-styles (2013-02-06) 1 commit
  (merged to 'next' on 2013-02-09 at c8cff17)
 + Update CodingGuidelines for Perl
 Add coding guidelines for writing Perl scripts for Git.
 Will merge to 'master'.
* al/mergetool-printf-fix (2013-02-08) 1 commit
 - git-mergetool: print filename when it contains %
 Will merge to 'next'.
* jk/error-const-return (2013-02-08) 1 commit
 - Use __VA_ARGS__ for all of error's arguments
 Will merge to 'next'.
* mm/allow-contrib-build (2013-02-07) 2 commits
 - perl.mak: introduce $(GIT_ROOT_DIR) to allow inclusion from other directories
 - Makefile: extract perl-related rules to make them available from other dirs
 Will merge to 'next'.
* mm/remote-mediawiki-build (2013-02-08) 2 commits
 - git-remote-mediawiki: use toplevel's Makefile
 - Makefile: make script-related rules usable from subdirectories
 Will merge to 'next'.
* nd/branch-show-rebase-bisect-state (2013-02-08) 1 commit
 - branch: show rebase/bisect info when possible instead of "(no branch)"
 Will merge to 'next'.
* nd/count-garbage (2013-02-08) 3 commits
 - count-objects: report how much disk space taken by garbage files
 - count-objects: report garbage files in pack directory too
 - git-count-objects.txt: describe each line in -v output
 Expecting a reroll.
 $gmane/215843
* wk/gc-auto-is-available-these-days (2013-02-08) 1 commit
 - user-manual: Rewrite git-gc section for automatic packing
 Will merge to 'next'.
* wk/man-deny-current-branch-is-default-these-days (2013-02-08) 1 commit
 - user-manual: Update for receive.denyCurrentBranch=refuse
 Will merge to 'next'.
* bw/get-tz-offset-perl (2013-02-09) 3 commits
 - cvsimport: format commit timestamp ourselves without using strftime
 - perl/Git.pm: fix get_tz_offset to properly handle DST boundary cases
 - Move Git::SVN::get_tz to Git::get_tz_offset
 Will merge to 'next'.
* mg/bisect-doc (2013-02-09) 1 commit
 - git-bisect.txt: clarify that reset finishes bisect
 Will merge to 'next'.

-------------------------------------------------- [Stalled]

* mp/diff-algo-config (2013-01-16) 3 commits
 - diff: Introduce --diff-algorithm command line option
 - config: Introduce diff.algorithm variable
 - git-completion.bash: Autocomplete --minimal and --histogram for git-diff
 Add diff.algorithm configuration so that the user does not type
 "diff --histogram".
 Looking better; may want tests to protect it from future breakages,
 but otherwise it looks ready for 'next'.
 Expecting a follow-up to add tests.
* mb/gitweb-highlight-link-target (2012-12-20) 1 commit
 - Highlight the link target line in Gitweb using CSS
 Expecting a reroll.
 $gmane/211935
* jk/lua-hackery (2012-10-07) 6 commits
 - pretty: fix up one-off format_commit_message calls
 - Minimum compilation fixup
 - Makefile: make "lua" a bit more configurable
 - add a "lua" pretty format
 - add basic lua infrastructure
 - pretty: make some commit-parsing helpers more public
 Interesting exercise. When we do this for real, we probably would want
 to wrap a commit to make it more like an "object" with methods like
 "parents", etc.
* rc/maint-complete-git-p4 (2012-09-24) 1 commit
 - Teach git-completion about git p4
 Comment from Pete will need to be addressed ($gmane/206172).
* jc/maint-name-rev (2012-09-17) 7 commits
 - describe --contains: use "name-rev --algorithm=weight"
 - name-rev --algorithm=weight: tests and documentation
 - name-rev --algorithm=weight: cache the computed weight in notes
 - name-rev --algorithm=weight: trivial optimization
 - name-rev: --algorithm option
 - name_rev: clarify the logic to assign a new tip-name to a commit
 - name-rev: lose unnecessary typedef
 "git name-rev" names the given revision based on a ref that can be
 reached in the smallest number of steps from the rev, but that is
 not useful when the caller wants to know which tag is the oldest one
 that contains the rev.  This teaches a new mode to the command that
 uses the oldest ref among those which contain the rev.
 I am not sure if this is worth it; for one thing, even with the help
 from notes-cache, it seems to make the "describe --contains" even
 slower. Also the command will be unusably slow for a user who does
 not have a write access (hence unable to create or update the
 notes-cache).
 Stalled mostly due to lack of responses.
* jc/xprm-generation (2012-09-14) 1 commit
 - test-generation: compute generation numbers and clock skews
 A toy to analyze how bad the clock skews are in histories of real
 world projects.
 Stalled mostly due to lack of responses.
* jc/add-delete-default (2012-08-13) 1 commit
 - git add: notice removal of tracked paths by default
 "git add dir/" updated modified files and added new files, but does
 not notice removed files, which may be "Huh?" to some users.  They
 can of course use "git add -A dir/", but why should they?
 Resurrected from graveyard, as I thought it was a worthwhile thing
 to do in the longer term.
 Stalled mostly due to lack of responses.
* mb/remote-default-nn-origin (2012-07-11) 6 commits
 - Teach get_default_remote to respect remote.default.
 - Test that plain "git fetch" uses remote.default when on a detached HEAD.
 - Teach clone to set remote.default.
 - Teach "git remote" about remote.default.
 - Teach remote.c about the remote.default configuration setting.
 - Rename remote.c's default_remote_name static variables.
 When the user does not specify what remote to interact with, we
 often attempt to use 'origin'.  This can now be customized via a
 configuration variable.
 Expecting a reroll.
 $gmane/210151
 "The first remote becomes the default" bit is better done as a
 separate step.
* nd/parse-pathspec (2013-01-11) 20 commits
 . Convert more init_pathspec() to parse_pathspec()
 . Convert add_files_to_cache to take struct pathspec
 . Convert {read,fill}_directory to take struct pathspec
 . Convert refresh_index to take struct pathspec
 . Convert report_path_error to take struct pathspec
 . checkout: convert read_tree_some to take struct pathspec
 . Convert unmerge_cache to take struct pathspec
 . Convert read_cache_preload() to take struct pathspec
 . add: convert to use parse_pathspec
 . archive: convert to use parse_pathspec
 . ls-files: convert to use parse_pathspec
 . rm: convert to use parse_pathspec
 . checkout: convert to use parse_pathspec
 . rerere: convert to use parse_pathspec
 . status: convert to use parse_pathspec
 . commit: convert to use parse_pathspec
 . clean: convert to use parse_pathspec
 . Export parse_pathspec() and convert some get_pathspec() calls
 . Add parse_pathspec() that converts cmdline args to struct pathspec
 . pathspec: save the non-wildcard length part
 Uses the parsed pathspec structure in more places where we used to
 use the raw "array of strings" pathspec.
 Ejected from 'pu' for now; will take a look at the rerolled one
 later ($gmane/213340).

-------------------------------------------------- [Cooking]

* jc/extended-fake-ancestor-for-gitlink (2013-02-05) 1 commit
  (merged to 'next' on 2013-02-09 at 2d3547b)
 + apply: verify submodule commit object name better
 Instead of requiring the full 40-hex object names on the index
 line, we can read submodule commit object names from the textual
 diff when synthesizing a fake ancestore tree for "git am -3".
 Will merge to 'master'.
* tz/credential-authinfo (2013-02-05) 1 commit
 - Add contrib/credentials/netrc with GPG support
 A new read-only credential helper (in contrib/) to interact with
 the .netrc/.authinfo files.  Hopefully mn/send-email-authinfo topic
 can rebuild on top of something like this.
 Expecting a reroll.
 $gmane/215556
* jx/utf8-printf-width (2013-02-09) 1 commit
 - Add utf8_fprintf helper that returns correct number of columns
 Use a new helper that prints a message and counts its display width
 to align the help messages parse-options produces.
 Will merge to 'next'.
* dg/subtree-fixes (2013-02-05) 6 commits
  (merged to 'next' on 2013-02-09 at 8f19ebe)
 + contrib/subtree: make the manual directory if needed
 + contrib/subtree: honor DESTDIR
 + contrib/subtree: fix synopsis
 + contrib/subtree: better error handling for 'subtree add'
 + contrib/subtree: use %B for split subject/body
 + contrib/subtree: remove test number comments
 contrib/subtree updates, but here are only the ones that looked
 ready to be merged to 'next'.  For the remainder, they will have
 another day.
 Will merge to 'master'.
* jl/submodule-deinit (2013-02-06) 1 commit
 - submodule: add 'deinit' command
 There was no Porcelain way to say "I no longer am interested in
 this submodule", once you express your interest in a submodule with
 "submodule init".  "submodule deinit" is the way to do so.
 Will merge to 'next'.
* jc/remove-export-from-config-mak-in (2013-02-03) 1 commit
  (merged to 'next' on 2013-02-07 at 33f7d4f)
 + config.mak.in: remove unused definitions
 config.mak.in template had an "export" line to cause a few
 common makefile variables to be exported; if they need to be
 expoted for autoconf/configure users, they should also be exported
 for people who write config.mak the same way.  Move the "export" to
 the main Makefile.
* nd/status-show-in-progress (2013-02-05) 1 commit
 - status: show the branch name if possible in in-progress info
 Will merge to 'next'.
* sp/smart-http-content-type-check (2013-02-06) 3 commits
  (merged to 'next' on 2013-02-06 at 8bc6434)
 + http_request: reset "type" strbuf before adding
  (merged to 'next' on 2013-02-05 at 157812c)
 + t5551: fix expected error output
  (merged to 'next' on 2013-02-04 at d0759cb)
 + Verify Content-Type from smart HTTP servers
 The smart HTTP clients forgot to verify the content-type that comes
 back from the server side to make sure that the request is being
 handled properly.
 Will merge to 'master'.
* jc/mention-tracking-for-pull-default (2013-01-31) 1 commit
 - doc: mention tracking for pull.default
 We stopped mentioning `tracking` is a deprecated but supported
 synonym for `upstream` in pull.default even though we have no
 intention of removing the support for it.
 This is my "don't list it to catch readers' eyes, but make sure it
 can be found if the reader looks for it" version; I'm not married
 to the layout and will be happy to take a replacement patch.
 Waiting for couter-proposal patches.
* jc/hidden-refs (2013-02-07) 3 commits
 - upload/receive-pack: allow hiding ref hierarchies
 - upload-pack: simplify request validation
 - upload-pack: share more code
 (this branch is used by jc/fetch-raw-sha1.)
 Allow the server side to redact the refs/ namespace it shows to the
 client.
 I think this is ready for 'next'.
* jc/remove-treesame-parent-in-simplify-merges (2013-01-17) 1 commit
  (merged to 'next' on 2013-01-30 at b639b47)
 + simplify-merges: drop merge from irrelevant side branch
 The --simplify-merges logic did not cull irrelevant parents from a
 merge that is otherwise not interesting with respect to the paths
 we are following.
 This touches a fairly core part of the revision traversal
 infrastructure; even though I think this change is correct, please
 report immediately if you find any unintended side effect.
 Will cook in 'next'.
* jc/push-2.0-default-to-simple (2013-01-16) 14 commits
  (merged to 'next' on 2013-01-16 at 23f5df2)
 + t5570: do not assume the "matching" push is the default
 + t5551: do not assume the "matching" push is the default
 + t5550: do not assume the "matching" push is the default
  (merged to 'next' on 2013-01-09 at 74c3498)
 + doc: push.default is no longer "matching"
 + push: switch default from "matching" to "simple"
 + t9401: do not assume the "matching" push is the default
 + t9400: do not assume the "matching" push is the default
 + t7406: do not assume the "matching" push is the default
 + t5531: do not assume the "matching" push is the default
 + t5519: do not assume the "matching" push is the default
 + t5517: do not assume the "matching" push is the default
 + t5516: do not assume the "matching" push is the default
 + t5505: do not assume the "matching" push is the default
 + t5404: do not assume the "matching" push is the default
 Will cook in 'next' until Git 2.0 ;-).
* bc/append-signed-off-by (2013-01-27) 11 commits
 - Unify appending signoff in format-patch, commit and sequencer
 - format-patch: update append_signoff prototype
 - t4014: more tests about appending s-o-b lines
 - sequencer.c: teach append_signoff to avoid adding a duplicate newline
 - sequencer.c: teach append_signoff how to detect duplicate s-o-b
 - sequencer.c: always separate "(cherry picked from" from commit body
 - sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
 - t/t3511: add some tests of 'cherry-pick -s' functionality
 - t/test-lib-functions.sh: allow to specify the tag name to test_commit
 - commit, cherry-pick -s: remove broken support for multiline rfc2822 fields
 - sequencer.c: rework search for start of footer to improve clarity
 Waiting for the final round of reroll before merging to 'next'.
 After that we will go incremental.

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

* mn/send-email-authinfo (2013-01-29) 1 commit
 . git-send-email: add ~/.authinfo parsing
 Instead of making send-email directly read from .netrc/.authinfo,
 mn/send-email-works-with-credential topic hooks the program to our
 credential framework, and tz/credential-authinfo topic gives access
 to these file formats to credential consumers.
John Keeping· Feb 10, 2013, 13:16 UTC · re: Junio C Hamano · lore

[PATCH] fixup! graph: output padding for merge subsequent parents

--- On Sat, Feb 09, 2013 at 03:39:33PM -0800, Junio C Hamano wrote:

Show 12 quoted lines
> * jk/diff-graph-cleanup (2013-02-07) 6 commits
>  - combine-diff.c: teach combined diffs about line prefix
>  - diff.c: use diff_line_prefix() where applicable
>  - diff: add diff_line_prefix function
>  - diff.c: make constant string arguments const
>  - diff: write prefix to the correct file
>  - graph: output padding for merge subsequent parents
> 
>  Refactors a lot of repetitive code sequence from the graph drawing
>  code and adds it to the combined diff output.
> 
>  Will merge to 'next'.
Can you squash this into the first commit before you do?

Matthieu is correct that the graph_is_commit_finished() check isn't needed in the loop now that we've pulled it out to be checked first - the value returned can't change during the loop. I've left the early return out.

 graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/graph.c b/graph.c
index 2a3fc5c..56f970f 100644
--- a/graph.c
+++ b/graph.c
@@ -1237,7 +1237,7 @@ void graph_show_commit(struct git_graph *graph)
 		shown_commit_line = 1;
 	}
 
-	while (!shown_commit_line && !graph_is_commit_finished(graph)) {
+	while (!shown_commit_line) {
 		shown_commit_line = graph_next_line(graph, &msgbuf);
 		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
 		if (!shown_commit_line)
-- 
1.8.1.2
Junio C Hamano· Feb 10, 2013, 19:30 UTC · re: John Keeping · lore

Re: [PATCH] fixup! graph: output padding for merge subsequent parents

John Keeping <john@keeping.me.uk> writes:
Show 23 quoted lines
> Can you squash this into the first commit before you do?
>
> Matthieu is correct that the graph_is_commit_finished() check isn't
> needed in the loop now that we've pulled it out to be checked first -
> the value returned can't change during the loop.  I've left the early
> return out.
>
>  graph.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/graph.c b/graph.c
> index 2a3fc5c..56f970f 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -1237,7 +1237,7 @@ void graph_show_commit(struct git_graph *graph)
>  		shown_commit_line = 1;
>  	}
>  
> -	while (!shown_commit_line && !graph_is_commit_finished(graph)) {
> +	while (!shown_commit_line) {
>  		shown_commit_line = graph_next_line(graph, &msgbuf);
>  		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
>  		if (!shown_commit_line)

Is it correct to say that this essentially re-does 656197ad3805 (graph.c: infinite loop in git whatchanged --graph -m, 2009-07-25) in a slightly different way, in that Michał's original fix also protected against the case where graph->state is flipped to GRAPH_PADDING by graph_next_line() that returns false, but with your fixup, the code knows it never happens (i.e. when graph_next_line() returns false, graph->state is always in the GRAPH_PADDING state), and the only thing we need to be careful about is when graph->state is already in the PADDING state upon entry to this function?

Sorry for an overlong single sentence question ;-)
Thanks.
John Keeping· Feb 10, 2013, 21:02 UTC · re: Junio C Hamano · lore

Re: [PATCH] fixup! graph: output padding for merge subsequent parents

On Sun, Feb 10, 2013 at 11:30:39AM -0800, Junio C Hamano wrote:
Show 35 quoted lines
> John Keeping <john@keeping.me.uk> writes:
> 
> > Can you squash this into the first commit before you do?
> >
> > Matthieu is correct that the graph_is_commit_finished() check isn't
> > needed in the loop now that we've pulled it out to be checked first -
> > the value returned can't change during the loop.  I've left the early
> > return out.
> >
> >  graph.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/graph.c b/graph.c
> > index 2a3fc5c..56f970f 100644
> > --- a/graph.c
> > +++ b/graph.c
> > @@ -1237,7 +1237,7 @@ void graph_show_commit(struct git_graph *graph)
> >  		shown_commit_line = 1;
> >  	}
> >  
> > -	while (!shown_commit_line && !graph_is_commit_finished(graph)) {
> > +	while (!shown_commit_line) {
> >  		shown_commit_line = graph_next_line(graph, &msgbuf);
> >  		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
> >  		if (!shown_commit_line)
> 
> Is it correct to say that this essentially re-does 656197ad3805
> (graph.c: infinite loop in git whatchanged --graph -m, 2009-07-25)
> in a slightly different way, in that Michał's original fix also
> protected against the case where graph->state is flipped to
> GRAPH_PADDING by graph_next_line() that returns false, but with your
> fixup, the code knows it never happens (i.e. when graph_next_line()
> returns false, graph->state is always in the GRAPH_PADDING state),
> and the only thing we need to be careful about is when graph->state
> is already in the PADDING state upon entry to this function?

Yes, although I wonder if we can end up in POST_MERGE or COLLAPSING state here as well. The check in the loop guards against that because those will eventually end up as PADDING.

As far as I can see, this is okay because we have called graph_show_remainder() at the end of outputting a commit, even when we end up outputting the same (merge) commit more than once. But someone more familiar with the graph code might want to comment here.

John
Junio C Hamano· Feb 10, 2013, 22:38 UTC · re: John Keeping · lore

Re: [PATCH] fixup! graph: output padding for merge subsequent parents

John Keeping <john@keeping.me.uk> writes:
Show 20 quoted lines
> On Sun, Feb 10, 2013 at 11:30:39AM -0800, Junio C Hamano wrote:
> ...
>> Is it correct to say that this essentially re-does 656197ad3805
>> (graph.c: infinite loop in git whatchanged --graph -m, 2009-07-25)
>> in a slightly different way, in that Michał's original fix also
>> protected against the case where graph->state is flipped to
>> GRAPH_PADDING by graph_next_line() that returns false, but with your
>> fixup, the code knows it never happens (i.e. when graph_next_line()
>> returns false, graph->state is always in the GRAPH_PADDING state),
>> and the only thing we need to be careful about is when graph->state
>> is already in the PADDING state upon entry to this function?
>
> Yes, although I wonder if we can end up in POST_MERGE or COLLAPSING
> state here as well.  The check in the loop guards against that because
> those will eventually end up as PADDING.
>
> As far as I can see, this is okay because we have called
> graph_show_remainder() at the end of outputting a commit, even when we
> end up outputting the same (merge) commit more than once.  But someone
> more familiar with the graph code might want to comment here.

More importantly, that kind of thought process needs to be documented in the log message; that will help people to diagnose the cause of the problem if they later find that this patch made an incorrect assumption while simplifying the code.

John Keeping· Feb 11, 2013, 10:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] fixup! graph: output padding for merge subsequent parents

On Sun, Feb 10, 2013 at 02:38:46PM -0800, Junio C Hamano wrote:
Show 26 quoted lines
> John Keeping <john@keeping.me.uk> writes:
> > On Sun, Feb 10, 2013 at 11:30:39AM -0800, Junio C Hamano wrote:
> > ...
> >> Is it correct to say that this essentially re-does 656197ad3805
> >> (graph.c: infinite loop in git whatchanged --graph -m, 2009-07-25)
> >> in a slightly different way, in that Michał's original fix also
> >> protected against the case where graph->state is flipped to
> >> GRAPH_PADDING by graph_next_line() that returns false, but with your
> >> fixup, the code knows it never happens (i.e. when graph_next_line()
> >> returns false, graph->state is always in the GRAPH_PADDING state),
> >> and the only thing we need to be careful about is when graph->state
> >> is already in the PADDING state upon entry to this function?
> >
> > Yes, although I wonder if we can end up in POST_MERGE or COLLAPSING
> > state here as well.  The check in the loop guards against that because
> > those will eventually end up as PADDING.
> >
> > As far as I can see, this is okay because we have called
> > graph_show_remainder() at the end of outputting a commit, even when we
> > end up outputting the same (merge) commit more than once.  But someone
> > more familiar with the graph code might want to comment here.
> 
> More importantly, that kind of thought process needs to be
> documented in the log message; that will help people to diagnose the
> cause of the problem if they later find that this patch made an
> incorrect assumption while simplifying the code.

Perhaps it's best to leave the patch as it originally was to guarantee that we can't get stuck in graph_show_commit(), even when it's called at an unexpected time, but I see you've already squashed this change in.

Would you prefer me to resend the original patch or send an update with this change and the above reasoning in the commit message?

John
Junio C Hamano· Feb 11, 2013, 16:42 UTC · re: John Keeping · lore

Re: [PATCH] fixup! graph: output padding for merge subsequent parents

John Keeping <john@keeping.me.uk> writes:
Show 6 quoted lines
> Perhaps it's best to leave the patch as it originally was to guarantee
> that we can't get stuck in graph_show_commit(), even when it's called at
> an unexpected time, but I see you've already squashed this change in.
>
> Would you prefer me to resend the original patch or send an update with
> this change and the above reasoning in the commit message?

Yes, please. Let's have the original (I think I have it in my reflog so no need to resend it) and this update on top as a separate patch with an updated log message.

John Keeping· Feb 11, 2013, 19:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] fixup! graph: output padding for merge subsequent parents

On Mon, Feb 11, 2013 at 08:42:21AM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> John Keeping <john@keeping.me.uk> writes:
> 
> > Perhaps it's best to leave the patch as it originally was to guarantee
> > that we can't get stuck in graph_show_commit(), even when it's called at
> > an unexpected time, but I see you've already squashed this change in.
> >
> > Would you prefer me to resend the original patch or send an update with
> > this change and the above reasoning in the commit message?
> 
> Yes, please.  Let's have the original (I think I have it in my
> reflog so no need to resend it) and this update on top as a separate
> patch with an updated log message.

I was suggesting dropping the change to remove the graph_is_commit_finished() check in the loop. I'm not sure it buys us much and there are still situations that could result in the state changing to PADDING during the loop if the graph API is used in an unexpected way.

Are others convinced that this change is always safe?
John
Junio C Hamano· Feb 11, 2013, 19:58 UTC · re: John Keeping · lore

Re: [PATCH] fixup! graph: output padding for merge subsequent parents

John Keeping <john@keeping.me.uk> writes:
Show 19 quoted lines
> On Mon, Feb 11, 2013 at 08:42:21AM -0800, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>> 
>> > Perhaps it's best to leave the patch as it originally was to guarantee
>> > that we can't get stuck in graph_show_commit(), even when it's called at
>> > an unexpected time, but I see you've already squashed this change in.
>> >
>> > Would you prefer me to resend the original patch or send an update with
>> > this change and the above reasoning in the commit message?
>> 
>> Yes, please.  Let's have the original (I think I have it in my
>> reflog so no need to resend it) and this update on top as a separate
>> patch with an updated log message.
>
> I was suggesting dropping the change to remove the
> graph_is_commit_finished() check in the loop.  I'm not sure it buys us
> much and there are still situations that could result in the state
> changing to PADDING during the loop if the graph API is used in an
> unexpected way.

OK, so the fixup! was not done with enough thought. I am fine with dropping it.

Thanks.
Matthieu Moy· Feb 11, 2013, 09:14 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Feb 2013, #04; Sat, 9)

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> * mm/allow-contrib-build (2013-02-07) 2 commits
>  - perl.mak: introduce $(GIT_ROOT_DIR) to allow inclusion from other directories
>  - Makefile: extract perl-related rules to make them available from other dirs
>
>  Will merge to 'next'.
These two patches do not make much sense without the 3rd one:
  [PATCH 3/4] Makefile: factor common configuration in git-default-config.mak

because perl.mak uses $(pathsep) that is defined in Makefile, hence unreachable from external callers.

You can either drop the series (I still think it's a good thing to make the toplevel Makefile more modular, but I have no longer a personal interest in it since the original goal is already reached by mm/remote-mediawiki-build), or continue the discussion on [PATCH 3/4].

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Feb 11, 2013, 16:01 UTC · re: Matthieu Moy · lore

Re: What's cooking in git.git (Feb 2013, #04; Sat, 9)

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 19 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> * mm/allow-contrib-build (2013-02-07) 2 commits
>>  - perl.mak: introduce $(GIT_ROOT_DIR) to allow inclusion from other directories
>>  - Makefile: extract perl-related rules to make them available from other dirs
>>
>>  Will merge to 'next'.
>
> These two patches do not make much sense without the 3rd one:
>
>   [PATCH 3/4] Makefile: factor common configuration in git-default-config.mak
>
> because perl.mak uses $(pathsep) that is defined in Makefile, hence
> unreachable from external callers.
>
> You can either drop the series (I still think it's a good thing to make
> the toplevel Makefile more modular, but I have no longer a personal
> interest in it since the original goal is already reached by
> mm/remote-mediawiki-build), or continue the discussion on [PATCH 3/4].

Ahh, you are absolutely right. Thanks for reminding me.

← back to recent threads