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

Re: [PATCH v2 0/5] roll back locks in various code paths

From
Jeff King <peff@peff.net>
Date
Mar 1, 2018, 07:41 UTC
Message-ID
<20180301074129.GE31079@sigill.intra.peff.net>
In-Reply-To
<20180228195809.9145-1-martin.agren@gmail.com>
On Wed, Feb 28, 2018 at 08:58:09PM +0100, Martin Ågren wrote:
Show 10 quoted lines
> > I'll follow up with a patch to
> > address the confusing pattern which Peff mentioned and which fooled me
> > when I prepared v1.
> 
> Here is such a patch on top of the others. I'm not particularly proud of the
> name SKIP_IF_UNCHANGED, but don't think it's any worse than, e.g.,
> IGNORE_UNCHANGED or NO_UNCHANGED_WRITE. :-/ Suggestions welcome.
> 
> I think this makes the current users a bit more obvious, and should help future
> users get this optimization right.
IMHO the result is easier to follow. Except for one case:
Show 18 quoted lines
> -	if (active_cache_changed || force_write) {
> -		if (newfd < 0) {
> -			if (refresh_args.flags & REFRESH_QUIET)
> -				exit(128);
> -			unable_to_lock_die(get_index_file(), lock_error);
> -		}
> -		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> -			die("Unable to write new index file");
> +	if (newfd < 0 && (active_cache_changed || force_write)) {
> +		if (refresh_args.flags & REFRESH_QUIET)
> +			exit(128);
> +		unable_to_lock_die(get_index_file(), lock_error);
>  	}
>  
> -	rollback_lock_file(&lock_file);
> +	if (write_locked_index(&the_index, &lock_file,
> +			       COMMIT_LOCK | (force_write ? 0 : SKIP_IF_UNCHANGED)))
> +		die("Unable to write new index file");

where I think the logic just ends up repeating itself. I guess you were anxious to try to get rid of active_cached_changed, but I don't think keeping it around is really that big a deal (and certainly another trick is to just say "the_index.cache_changed").

-Peff
Previous: Martin ÅgrenNext: Junio C Hamano
Message 18 of 26 in “roll back locks in various code paths”
  1. 0/5 roll back locks in various code pathsMartin Ågren, Feb 27, 2018
  2. 1/5 sequencer: make lockfiles non-staticMartin Ågren, Feb 27, 2018
  3. 2/5 sequencer: always roll back lock in `do_recursive_merge()`Martin Ågren, Feb 27, 2018
  4. Jeff KingFeb 27, 2018
  5. Martin ÅgrenFeb 27, 2018
  6. Jeff KingFeb 27, 2018
  7. 3/5 merge-recursive: always roll back lock in `merge_recursive_generic()`Martin Ågren, Feb 27, 2018
  8. Martin ÅgrenFeb 28, 2018
  9. 0/5 roll back locks in various code pathsMartin Ågren, Feb 28, 2018
  10. 1/5 sequencer: make lockfiles non-staticMartin Ågren, Feb 28, 2018
  11. 2/5 sequencer: always roll back lock in `do_recursive_merge()`Martin Ågren, Feb 28, 2018
  12. 5/5 sequencer: do not roll back lockfile unnecessarilyMartin Ågren, Feb 28, 2018
  13. 4/5 merge: always roll back lock in `checkout_fast_forward()`Martin Ågren, Feb 28, 2018
  14. 3/5 merge-recursive: always roll back lock in `merge_recursive_generic()`Martin Ågren, Feb 28, 2018
  15. Martin ÅgrenFeb 28, 2018
  16. Junio C HamanoFeb 28, 2018
  17. Martin ÅgrenMar 1, 2018
  18. Jeff KingMar 1, 2018
  19. Junio C HamanoMar 1, 2018
  20. Martin ÅgrenMar 1, 2018
  21. Jeff KingMar 2, 2018
  22. Jeff KingMar 1, 2018
  23. 4/5 merge: always roll back lock in `checkout_fast_forward()`Martin Ågren, Feb 27, 2018
  24. 5/5 sequencer: do not roll back lockfile unnecessarilyMartin Ågren, Feb 27, 2018
  25. Jeff KingFeb 27, 2018
  26. Junio C HamanoFeb 27, 2018

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.