git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 02/31] rebase: refactor reading of state

From
Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
Date
Dec 29, 2010, 08:09 UTC
Message-ID
<alpine.DEB.1.10.1012290550080.29381@debian>
In-Reply-To
<7v1v51v6l2.fsf@alter.siamese.dyndns.org>
On Tue, 28 Dec 2010, Junio C Hamano wrote:
Show 37 quoted lines
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
> 
> > +read_state () {
> > +	if test -d "$merge_dir"
> > +	then
> > +		state_dir="$merge_dir"
> > +		prev_head=$(cat "$merge_dir"/prev_head) &&
> > +		end=$(cat "$merge_dir"/end) &&
> > +		msgnum=$(cat "$merge_dir"/msgnum)
> > +	else
> > +		state_dir="$apply_dir"
> > +	fi &&
> > +	head_name=$(cat "$state_dir"/head-name) &&
> > +	onto=$(cat "$state_dir"/onto) &&
> > +	orig_head=$(cat "$state_dir"/orig-head) &&
> > +	GIT_QUIET=$(cat "$state_dir"/quiet)
> > +}
> > +
> >  continue_merge () {
> >  	test -n "$prev_head" || die "prev_head must be defined"
> >  	test -d "$merge_dir" || die "$merge_dir directory does not exist"
> > @@ -138,10 +154,6 @@ call_merge () {
> >  }
> >  
> >  move_to_original_branch () {
> > -	test -z "$head_name" &&
> > -		head_name="$(cat "$merge_dir"/head-name)" &&
> > -		onto="$(cat "$merge_dir"/onto)" &&
> > -		orig_head="$(cat "$merge_dir"/orig-head)"
> 
> It used to be safe to call this without head_name and friends set, because
> the function took care of reading these itself.  Nobody calls this without
> head_name set anymore?
> 
> I am not complaining nor suggesting to add an unnecessary "read_state"
> here only to slow things down---I am making sure that you removed this
> because you know it is unnecessary.

Correct. It used to be called without head_name set from finish_rb_merge, which would in turn be called from the --continue and --skip arms. onto would already have been set in these cases, but would then be re-read.

Show 17 quoted lines
> > @@ -220,13 +232,9 @@ do
> >  			echo "mark them as resolved using git add"
> >  			exit 1
> >  		}
> > +		read_state
> >  		if test -d "$merge_dir"
> >  		then
> > -			prev_head=$(cat "$merge_dir/prev_head")
> > -			end=$(cat "$merge_dir/end")
> > -			msgnum=$(cat "$merge_dir/msgnum")
> > -			onto=$(cat "$merge_dir/onto")
> > -			GIT_QUIET=$(cat "$merge_dir/quiet")
> 
> Even though this patch may make the code shorter, it starts to read
> head_name and orig_head that we previously did not open and change the
> values of variables with what are read from them.  Does this change affect
> the behaviour in any way (either in performance or in correctness)?

True. Previously, head_name and orig_head were lazily read only when the rebase had been finished (in the finish_rb_merge, as I mentioned above). Now these values are read unnecessarily when the rebase is resumed, but a later patch fails. The am-based rebase is not affacted by this patch as it already read head_name and orig_head eagerly.

Good point about the correctness. I looked into the correctness issue arising if a file could not be found and I think I concluded that all of the files would always be written (do we need to care about the case where a user deletes e.g. .git/rebase-apply/onto?). However, I did not think about the possibility of overwriting variables. It seems fine, though, as neither continue_merge nor call_merge uses either of these variables.

Regarding performance, I would say there is definitely a cost associated with this patch. How big this cost is, though, I don't dare to speculate. I will leave that up to the rest of you.

It should be noted that read_state is only called when a rebase is resumed with --continue or --skip, or when it is aborted. There are no changes to the code in the call_merge-continue_merge loop.

The performance hit is probably biggest in the --abort case, which previously only read head_name and orig_head. It now ends up reading _at least_ two more values.

Show 17 quoted lines
> 
> > @@ -236,10 +244,6 @@ do
> >  			finish_rb_merge
> >  			exit
> >  		fi
> > -		head_name=$(cat "$apply_dir"/head-name) &&
> > -		onto=$(cat "$apply_dir"/onto) &&
> > -		orig_head=$(cat "$apply_dir"/orig-head) &&
> > -		GIT_QUIET=$(cat "$apply_dir"/quiet)
> >  		git am --resolved --3way --resolvemsg="$RESOLVEMSG" &&
> >  		move_to_original_branch
> 
> Earlier move-to-original-branch was Ok to be called without head_name, and
> the old code here read from the file anyway, so it didn't matter, but now
> it seems that the first check and assignment you removed from the function
> may matter because this caller does not even read from head_name.  Are you
> sure about this change?
If I understand your question correctly, then yes, it is ok, because
of the previous hunk that calls read_state. That call is made
before/outside the if block for merge-based rebase, so the variables
are already set when this code is reached.
Show 20 quoted lines
> > @@ -279,18 +275,15 @@ do
> >  			die "No rebase in progress?"
> >  
> >  		git rerere clear
> > -
> > -		test -d "$merge_dir" || merge_dir="$apply_dir"
> 
> My heartbeat skipped when I first saw this.  Thanks to the previous
> commit, it was exposed that somebody reused $dotest that was only to be
> used when using merge machinery because the things left to be done in this
> codepath are common between the merge and apply, which is kind of sloppy,
> but that sloppiness is now gone ;-).
> 
> Are there places that read from individual files for states left after
> this patch, or read_state is the only interface to get to the states?  If
> the latter that would be a great news, and also would suggest that we may
> want to have a corresponding write_state function (and we may even want to
> make the state into a single file to reduce I/O---but that is a separate
> issue that can be done at the very end of the series if it turns out to be
> beneficial).

There are still a few other places where state is read, mainly in call_merge. It reads things that vary from iteration to iteration, such as a counter. I forgot to say in the commit message, but I tried to extract only the code that reads the initial state.

The write_state function actually is there, but it comes pretty late,
in patch 24. I don't remember why I added it so much later. I could
possibly move it closer to the beginning of this series.
 
> Of course it is fine if introduction of read_state is an attempt to catch
> most common cases, but it would reduce chances of mistake if the coverage
> were 100% (as opposed to 99.9%) hence this question.

Do you mean if all the state was read in the read_state function? I should say that the pieces of state that are read in read_state are not read anywhere else. But overall, the coverage is more like 60% or so.

Thanks for a thorough review. Many of these things should have been in the commit message. I need to get better at writing those...

Thanks, Martin

Previous: Junio C HamanoNext: Martin von Zweigbergk
Message 7 of 118 in “Refactor rebase”
  1. 00/31 Refactor rebaseMartin von Zweigbergk, Dec 28, 2010
  2. 01/31 rebase: clearer names for directory variablesMartin von Zweigbergk, Dec 28, 2010
  3. Junio C HamanoDec 28, 2010
  4. Martin von ZweigbergkDec 28, 2010
  5. 02/31 rebase: refactor reading of stateMartin von Zweigbergk, Dec 28, 2010
  6. Junio C HamanoDec 28, 2010
  7. Martin von ZweigbergkDec 29, 2010
  8. 03/31 rebase: read state outside loopMartin von Zweigbergk, Dec 28, 2010
  9. 04/31 rebase: remove unused rebase state 'prev_head'Martin von Zweigbergk, Dec 28, 2010
  10. 05/31 rebase: improve detection of rebase in progressMartin von Zweigbergk, Dec 28, 2010
  11. Junio C HamanoDec 28, 2010
  12. Martin von ZweigbergkDec 28, 2010
  13. Martin von ZweigbergkJan 12, 2011
  14. 06/31 rebase: act on command line outside parsing loopMartin von Zweigbergk, Dec 28, 2010
  15. 07/31 rebase: stricter check of standalone sub commandMartin von Zweigbergk, Dec 28, 2010
  16. 08/31 rebase: align variable namesMartin von Zweigbergk, Dec 28, 2010
  17. Thomas RastJan 4, 2011
  18. Martin von ZweigbergkJan 5, 2011
  19. 09/31 rebase: align variable contentMartin von Zweigbergk, Dec 28, 2010
  20. 10/31 rebase: factor out command line option processingMartin von Zweigbergk, Dec 28, 2010
  21. 11/31 rebase -i: remove now unnecessary directory checksMartin von Zweigbergk, Dec 28, 2010
  22. 12/31 rebase: reorder validation stepsMartin von Zweigbergk, Dec 28, 2010
  23. 13/31 rebase: factor out reference parsingMartin von Zweigbergk, Dec 28, 2010
  24. 14/31 rebase: factor out clean work tree checkMartin von Zweigbergk, Dec 28, 2010
  25. 15/31 rebase: factor out call to pre-rebase hookMartin von Zweigbergk, Dec 28, 2010
  26. 16/31 rebase -i: support --statMartin von Zweigbergk, Dec 28, 2010
  27. Johannes SchindelinDec 28, 2010
  28. Martin von ZweigbergkDec 28, 2010
  29. Junio C HamanoDec 28, 2010
  30. Johannes SchindelinDec 28, 2010
  31. 17/31 rebase: remove $branch as synonym for $orig_headMartin von Zweigbergk, Dec 28, 2010
  32. 18/31 rebase: extract merge code to new source fileMartin von Zweigbergk, Dec 28, 2010
  33. Johannes SixtDec 29, 2010
  34. Martin von ZweigbergkDec 29, 2010
  35. Thomas RastDec 31, 2010
  36. Martin von ZweigbergkDec 31, 2010
  37. 19/31 rebase: extract am code to new source fileMartin von Zweigbergk, Dec 28, 2010
  38. 20/31 rebase: show consistent conflict resolution hintMartin von Zweigbergk, Dec 28, 2010
  39. 21/31 rebase -i: align variable namesMartin von Zweigbergk, Dec 28, 2010
  40. 22/31 rebase: make -v a tiny bit more verboseMartin von Zweigbergk, Dec 28, 2010
  41. 23/31 rebase: factor out sub command handlingMartin von Zweigbergk, Dec 28, 2010
  42. 24/31 rebase: extract code for writing basic stateMartin von Zweigbergk, Dec 28, 2010
  43. Thomas RastJan 4, 2011
  44. Martin von ZweigbergkJan 5, 2011
  45. 25/31 rebase: remember verbose optionMartin von Zweigbergk, Dec 28, 2010
  46. 26/31 rebase: remember strategy and strategy optionsMartin von Zweigbergk, Dec 28, 2010
  47. Thomas RastJan 4, 2011
  48. Martin von ZweigbergkJan 5, 2011
  49. 27/31 rebase -m: remember allow_rerere_autoupdate optionMartin von Zweigbergk, Dec 28, 2010
  50. 28/31 rebase -m: don't print exit code 2 when merge failsMartin von Zweigbergk, Dec 28, 2010
  51. 29/31 git-rebase--am: remove unnecessary --3way optionMartin von Zweigbergk, Dec 28, 2010
  52. 30/31 rebase -i: don't read unused variable preserve_mergesMartin von Zweigbergk, Dec 28, 2010
  53. 31/31 rebase -i: remove unnecessary state rebase-rootMartin von Zweigbergk, Dec 28, 2010
  54. Thomas RastDec 28, 2010
  55. Martin von ZweigbergkDec 29, 2010
  56. Christian CouderDec 31, 2010
  57. Thomas RastJan 4, 2011
  58. Martin von ZweigbergkJan 5, 2011
  59. 00/31 refactor rebaseMartin von Zweigbergk, Feb 6, 2011
  60. 01/31 rebase: clearer names for directory variablesMartin von Zweigbergk, Feb 6, 2011
  61. 02/31 rebase: refactor reading of stateMartin von Zweigbergk, Feb 6, 2011
  62. 03/31 rebase: read state outside loopMartin von Zweigbergk, Feb 6, 2011
  63. 04/31 rebase: remove unused rebase state 'prev_head'Martin von Zweigbergk, Feb 6, 2011
  64. 05/31 rebase: improve detection of rebase in progressMartin von Zweigbergk, Feb 6, 2011
  65. 06/31 rebase: act on command line outside parsing loopMartin von Zweigbergk, Feb 6, 2011
  66. 07/31 rebase: stricter check of standalone sub commandMartin von Zweigbergk, Feb 6, 2011
  67. Jonathan NiederJul 1, 2011
  68. Martin von ZweigbergkJul 1, 2011
  69. Jonathan NiederJul 1, 2011
  70. Martin von ZweigbergkJul 6, 2011
  71. 08/31 rebase: align variable namesMartin von Zweigbergk, Feb 6, 2011
  72. 09/31 rebase: align variable contentMartin von Zweigbergk, Feb 6, 2011
  73. 10/31 rebase: factor out command line option processingMartin von Zweigbergk, Feb 6, 2011
  74. 11/31 rebase -i: remove now unnecessary directory checksMartin von Zweigbergk, Feb 6, 2011
  75. 12/31 rebase: reorder validation stepsMartin von Zweigbergk, Feb 6, 2011
  76. 13/31 rebase: factor out reference parsingMartin von Zweigbergk, Feb 6, 2011
  77. 14/31 rebase: factor out clean work tree checkMartin von Zweigbergk, Feb 6, 2011
  78. 15/31 rebase: factor out call to pre-rebase hookMartin von Zweigbergk, Feb 6, 2011
  79. 16/31 rebase -i: support --statMartin von Zweigbergk, Feb 6, 2011
  80. 17/31 rebase: remove $branch as synonym for $orig_headMartin von Zweigbergk, Feb 6, 2011
  81. 18/31 rebase: extract merge code to new source fileMartin von Zweigbergk, Feb 6, 2011
  82. Johannes SixtFeb 14, 2011
  83. Martin von ZweigbergkFeb 14, 2011
  84. Martin von ZweigbergkFeb 24, 2011
  85. Jeff KingFeb 24, 2011
  86. Jeff KingFeb 24, 2011
  87. Martin von ZweigbergkFeb 25, 2011
  88. Jeff KingFeb 25, 2011
  89. Junio C HamanoFeb 25, 2011
  90. Jakub NarebskiFeb 25, 2011
  91. Jeff KingMar 1, 2011
  92. Jakub NarebskiMar 1, 2011
  93. 19/31 rebase: extract am code to new source fileMartin von Zweigbergk, Feb 6, 2011
  94. 20/31 rebase: show consistent conflict resolution hintMartin von Zweigbergk, Feb 6, 2011
  95. 21/31 rebase -i: align variable namesMartin von Zweigbergk, Feb 6, 2011
  96. 22/31 rebase: make -v a tiny bit more verboseMartin von Zweigbergk, Feb 6, 2011
  97. 23/31 rebase: factor out sub command handlingMartin von Zweigbergk, Feb 6, 2011
  98. 24/31 rebase: extract code for writing basic stateMartin von Zweigbergk, Feb 6, 2011
  99. 25/31 rebase: remember verbose optionMartin von Zweigbergk, Feb 6, 2011
  100. 26/31 rebase: remember strategy and strategy optionsMartin von Zweigbergk, Feb 6, 2011
  101. 27/31 rebase -m: remember allow_rerere_autoupdate optionMartin von Zweigbergk, Feb 6, 2011
  102. 28/31 rebase -m: don't print exit code 2 when merge failsMartin von Zweigbergk, Feb 6, 2011
  103. 29/31 git-rebase--am: remove unnecessary --3way optionMartin von Zweigbergk, Feb 6, 2011
  104. 30/31 rebase -i: don't read unused variable preserve_mergesMartin von Zweigbergk, Feb 6, 2011
  105. 31/31 rebase -i: remove unnecessary state rebase-rootMartin von Zweigbergk, Feb 6, 2011
  106. Junio C HamanoFeb 10, 2011
  107. Martin von ZweigbergkFeb 12, 2011
  108. Martin von ZweigbergkFeb 14, 2011
  109. Martin von ZweigbergkFeb 14, 2011
  110. Junio C HamanoFeb 15, 2011
  111. Martin von ZweigbergkFeb 22, 2011
  112. Junio C HamanoFeb 22, 2011
  113. Martin von ZweigbergkFeb 23, 2011
  114. Johannes SixtFeb 16, 2011
  115. Martin von ZweigbergkFeb 17, 2011
  116. rebase: define options in OPTIONS_SPECMartin von Zweigbergk, Feb 24, 2011
  117. Junio C HamanoFeb 27, 2011
  118. Martin von ZweigbergkMar 1, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.