Git doesn't notice file has changed (Re: Found a bug in git 1.9.0 but can't reproduce it without copyrighted source code.)
(cc-ing msysgit list, where there are more Windows-knowledgeable people) yun sheng wrote: On Fri, Mar 28, 2014 at 9:40 AM, Jonathan Nieder jrnie...@gmail.com wrote: yun sheng wrote: these two files have the same timestamp, the same size, bug slightly different contents. How did they get the same timestamp? [...] Git I'm using is msysgit 1.9.0 on windows 7 Unixy operating systems have other fields like inode number and ctime that make it possible to notice that a file might have been changed without actually rereading it. Unfortunately Git for Windows is limited to what's in the WIN32_FILE_ATTRIBUTE_DATA which means the size, mtime, and mode are basically all it has to go by. Do you know of some other Windows API call that could help? The files get the same timestamp by using `git difftool -d` to view diffs, the diff tool I use id beyond compare 3, this command would generate temp files to feed the compare program, so these files get the same time stamp, I copied them out from the temp folder. I have no idea of the second quesiton, I am really not familiar with windows API. Do you mean this file may have been changed without rereading and git can't detect it? Sorry for the lack of clarity. I meant that normally git detects when a file might have been changed without actually reading the file. To do this, it uses heuristics like If all file attributes are unchanged, the file is unchanged which tend to work well on Unix. My question was whether there's some similar trick that could work better on Windows than the current code. For example, if some interested person ports something like Facebook's watchman[1] to Windows[2], then Git could take advantage of that work using something like Duy's file-watcher series[3], which would be one way to fix this problem. Thanks, Jonathan [1] https://github.com/facebook/watchman [2] using FindFirstChangeNotification and ReadDirectoryChangesW, perhaps [3] http://thread.gmane.org/gmane.comp.version-control.git/240339/focus=241395 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: fast-import deltas
Junio C Hamano wrote: Assuming that you do have and are willing to read the original file, you have three possible (and one impractical) approaches: [...] - Apply the foreign changes to the original file yourself, and feed the resulting content to fast-import in full, letting fast-import convert into the format Git understands. This (when importing from Subversion) was the motivation for introducing fast-import's cat-blob command, for what it's worth. [...] In short, the most practical solution would be to reconstitute a full object and feed that to fast-import, unless you already have xdelta or you can turn your foreign change into xdelta without ever looking at the original. If your delta format happens to be similar enough to xdelta, then streaming in deltas in xdelta format does sound like a neat trick. Maybe it would be useful to provide a micro-library that creates and validates xdelta opcodes for fast-import frontends to use. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-status -- trying to understand all possible states
Michael Toy wrote: https://gist.github.com/the-michael-toy/9907309 Two nits: - Please use --porcelain (implied by -z in the absence of another format option) instead of --short. --short is meant to be human readable and details of the output might change some day. - Depending on what you're trying to do, you might find the output from other commands such as git ls-files -s or git diff-files easier to work with. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'
(culling cc list) Hi, Christian Couder wrote: [Subject: Documentation: add documentation for 'git interpret-trailers'] Signed-off-by: Christian Couder chrisc...@tuxfamily.org This should be squashed into the patch that introduces the interpret-trailers command, IMHO (or if it should be reviewed separately, it can be an earlier patch). That way, someone looking at when the command was introduced and wanting to understand what it was originally meant to do has the information close by. Thanks for picking up the 'git commit --fixes' topic and your steady work improving the series. [...] --- /dev/null +++ b/Documentation/git-interpret-trailers.txt @@ -0,0 +1,123 @@ +git-interpret-trailers(1) += + +NAME + +git-interpret-trailers - help add stuctured information into commit messages + +SYNOPSIS + +[verse] +'git interpret-trailers' [--trim-empty] [(token[(=|:)value])...] + +DESCRIPTION +--- +Help add RFC 822-like headers, called 'trailers', at the end of the +otherwise free-form part of a commit message. + +This command is a filter. It reads the standard input for a commit +message and applies the `token` arguments, if any, to this +message. The resulting message is emited on the standard output. Do you have an example? Does it work like this? $ git interpret-trailers 'signoff=Jonathan Nieder jrnie...@gmail.com' EOF foo bar baz qux EOF foo bar baz qux Signed-off-by: Jonathan Nieder jrnie...@gmail.com $ A short EXAMPLES section could help. If I am understanding it correctly, would a name like 'git add-trailers' work? How do I read back the trailers later? [...] +By default, a 'token=value' or 'token:value' argument will be added +only if Why support both '=' and ':'? Using just one would make it easier to grep through scripts to see who is adding signoffs. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 0/7] fix hunk editing with 'commit -p -m'
Hi, A quick note for the future: Benoit Pierre wrote: This patch fixes the fact that hunk editing with 'commit -p -m' does not work: GIT_EDITOR is set to ':' to indicate to hooks that no editor will be launched, which result in the 'hunk edit' option not launching the editor (and selecting the whole hunk). This information should have gone in the relevant patch's commit message itself. That way, people don't have to hunt down the relevant mailing list thread to understand the patch. Generally a cover letter should say as little as possible (mostly here is what patch you might want to look at first, and here is an overview of why the patches are in this particular order). Thanks for a nice fix. Perhaps we'll see more in the future, hence this note. :) And if you have ideas for where an explanation of this could go in the documentation (somewhere in Documentation/SubmittingPatches?), that would be welcome too. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Patch Series v3 for use the $( ... ) construct for command substitution
Hi, Elia Pinto wrote: This patch series contain the use the $( ... ) construct for command substitution patches not already merged in ep/shell-command-substitution in the mantainer repository. Thanks for working on this. The $() form is less error-prone than ``, so in that sense it can be worthwhile. [...] Elia Pinto (140): I admit I'm not excited to review these at all. I wonder if it's possible to make the series easier to review. For example: * patch 0 makes preparatory changes to line wrapping or to avoid using `` some places to make an automatic transformation easier * patch 1 introduces a script to transform `` expressions to $() expressions * patch 2 just runs that script If the script is obviously correct enough then there is no need to manually go through 140 files after that point. If the only way to get this done is to actually manually review those 140 files, I just don't think it's worth it. The `` construct is not *that* bad. Another possible direction could be to add a tool to make sure git doesn't get any new uses of ``, to let the changes flow in at a manageable rate without too many cases of one step forward, one step back. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Patch Series v3 for use the $( ... ) construct for command substitution
Matthieu Moy wrote: Jonathan Nieder jrnie...@gmail.com writes: If the script is obviously correct enough then there is no need to manually go through 140 files after that point. The script cannot be obviously correct, as there are a lot of potential corner-cases (nested `, nesting ` within , comments, ...). To be a devil's advocate for a moment: * Comments are easy to detect. Remember, we are not trying to handle some adversary sending arbitrary well-formed shell scripts but just the git source code which already follows a consistent style. When I search for /#.*/ in .sh files, every match except for t/test-lib-functions.sh:output=`echo; echo # Stderr is:; cat $stderr` (which contains a backtick before the # mark) is a comment. * Nested ` is evil. A search for the string \' quickly finds them all, and they are very very rare. The only exception I see is git-svn tests, which independently of everything else is an obvious thing to fix first. * Nesting `` within double-quotes has the same semantics as $() within quotes. I don't think that poses a problem. [...] If the only way to get this done is to actually manually review those 140 files, I just don't think it's worth it. Honnestly, I went through the series once and it wasn't that painful. It is not just the people on the list reviewing now but others in the future wanting to understand whether it is safe to upgrade to a new version or where a bug they have run into came from. The simpler we can make the task of convincing oneself that the patch behaves as described, the better. 140 patches worth of churn once every couple of years is not terrible, but I really don't want to see this becoming a pattern. :/ And I don't see how the upside in this example warrants it. Hoping that clarifies, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Unicode: update of combining code points
Hi, Torsten Bögershausen wrote: Unicode 6.3 defines the following code as combining or accents, git_wcwidth() should return 0. Earlier unicode standards had defined these code point as reserved: Thanks for the update. Could the commit message also explain how this was noticed and what the user-visible effect is? For example: Unicode just announced that That means we should mark the relevant code points as combining characters so git knows they are zero-width and doesn't screw up the alignment when presenting branch names in columns with 'git branch --column' or something like that. [...] 358 COMBINING DOT ABOVE RIGHT 359 COMBINING ASTERISK BELOW I'm not sure this list is needed --- the code + the reference to the Unicode 6.3 standard seems like enough (but if you think otherwise, I don't really mind). This commit touches only the range 300-6FF, there may be more to be updated. The there may be more here sounds ominous. Does that mean Unicode 6.3 also added some zero-width characters in other ranges that should be dealt with in the future? How many such ranges? How do we know when we're done? Just biting off the most important characters first and putting off the rest for later sounds fine to me --- my complaint is that the above comment doesn't make clear what the to-do list is for finishing the update later. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Our official home page and logo for the Git project
Junio C Hamano wrote: In any case, this motion is not about let's declare the logo we see on git-scm.com today as _the_ official one. Phew. :) [...] Please help us by letting us answer Yup, that is a logo (among others) that represents our project, and we are OK with you using it to help promote our project instead. That is what I meant by our official logo in the first message. Sounds good to me. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] compat/regex/regcomp.c: reduce scope of variables
Hi, Elia Pinto wrote: [Subject: compat/regex/regcomp.c: reduce scope of variables] gnulib regex is still maintained upstream and available under the LGPL 2.1+. Shouldn't we make the change there and reimport to make it easier to pull in later changes? Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gitignore vs. exclude vs assume-unchanged?
Hi, a...@bellandwhistle.net wrote: In particular, 'exclude' is spottily documented. Where did you expect to read about it? I see some mention of .git/info/exclude in the gitignore(5) page, but I wouldn't be surprised if there's room for improvement there (improvements welcome). I realize the docs are structured strictly as an API reference, No, the docs are meant to be helpful, not just to confirm what people already know. ;-) Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Refactoring hardcoded SHA-1 constants
Hi, brian m. carlson wrote: I'd like to introduce a set of preprocessor constants that we'd use instead of hard-coded 20s and 40s everywhere. Lukewarm on that. It's hard to do consistently and unless they're named well it can be harder to know what something like BINARY_OBJECT_NAME_LENGTH means than plain '20' when first reading. [...] I would also like to consider, as a third step, turning all of the unsigned char[20] uses into a struct containing unsigned char[20] as its only member, like libgit2 does. That would be very welcome! It's a nice way to steer people toward hashcmp using the type system, and it makes it possible to use a union to enforce alignment later if measurements show benefit. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] Git v2.0.0-rc0
Kyle J. McKay wrote:
The problem with --prefix= is this (from the Getopt::Long CHANGES file):
Changes in version 2.37
---
* Bugfix: With gnu_compat, --foo= will no longer trigger Option
requires an argument but return the empty string.
The system I ran the tests on happens to have Getopt::Long version 2.35.
Thanks for catching it.
Getopt::Long was updated to 2.37 in perl 5.8.9 (in 5.8.8 it was
updated to 2.35). INSTALL still only recommends Perl 5.8 so that's an
issue.
The --prefix= option can be rewritten --prefix in both tests and then
they pass.
Hm, perhaps we should introduce a 'no-prefix' option to work around
this.
|diff --git a/git-svn.perl b/git-svn.perl
|index 7349ffea..284f458a 100755
|--- a/git-svn.perl
|+++ b/git-svn.perl
|@@ -149,7 +149,7 @@ my ($_trunk, @_tags, @_branches, $_stdlayout);
my %icv;
my %init_opts = ( 'template=s' = \$_template, 'shared:s' = \$_shared,
'trunk|T=s' = \$_trunk, 'tags|t=s@' = \@_tags,
'branches|b=s@' = \@_branches, 'prefix=s' = \$_prefix,
+ 'no-prefix' = sub { $_prefix = },
'stdlayout|s' = \$_stdlayout,
'minimize-url|m!' = \$Git::SVN::_minimize_url,
'no-metadata' = sub { $icv{noMetadata} = 1 },
That way, normal usage of --prefix would still be consistent with
other git commands that prefer the form with argument attached
(--prefix=foo, not --prefix foo; see gitcli(7)).
Thoughts?
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] Git v2.0.0-rc0
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Hm, perhaps we should introduce a 'no-prefix' option to work around this. [...] That way, normal usage of --prefix would still be consistent with other git commands that prefer the form with argument attached (--prefix=foo, not --prefix foo; see gitcli(7)). Thoughts? I do not think that it is a good idea to use --no-anything for something that is not a boolean. Do you mean it is a bad idea to support or a bad idea to make use of such support? I suggested --no- for consistency with current git commands that use parseopt. But on second thought, I agree that it be confusing for --prefix=foo --no-prefix to mean something different from no --prefix parameter at all. The documentation says --prefix=prefix ... Before Git 2.0, the default prefix was (no prefix). This meant that ... which suggests that I can use --prefix= to mean no prefix. Perhaps it needs a note to suggest using '--prefix ' instead? Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] Git v2.0.0-rc0
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: The documentation says --prefix=prefix ... Before Git 2.0, the default prefix was (no prefix). This meant that ... which suggests that I can use --prefix= to mean no prefix. Perhaps it needs a note to suggest using '--prefix ' instead? Is there another --option that takes an arbitrary user string that could be an empty string (or will there be one in the future)? In git in general, yes --- for example, 'git diff --src-prefix= HEAD^' tells git diff to leave off the usual c/ prefix in the src filename it prints. In git-svn, --trunk= or --message= might be meaningful, but not nearly as much as --prefix=. If that is the case, a better alternative might be to add an comment to say that those with older Getopt::Long may have to use --option instead of the --option= form for any option whose value happens to be an empty string to work around the command parser bug. Another possibility would be to require Perl 5.8.9 or newer. It was released in 2008. diff --git i/git-svn.perl w/git-svn.perl index 0a32372..ec7910d 100755 --- i/git-svn.perl +++ w/git-svn.perl @@ -1,7 +1,7 @@ #!/usr/bin/perl # Copyright (C) 2006, Eric Wong normalper...@yhbt.net # License: GPL v2 or later -use 5.008; +use 5.008_009; use warnings; use strict; use vars qw/ $AUTHOR $VERSION -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Archiving off old branches
Hi, Tim Chase wrote: cd .git/refs mkdir -p closed mv heads/BUG-123 closed That breaks with packed refs (see git-pack-refs(1)), which are a normal thing to encounter after garbage collection. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Trying to setup Visual Studio 2013 with a CentOS 6.5 x64 git server
Hi, Charles Buege wrote: If, in actuality, I can use a CentOS git server with Visual Studio 2013, can anyone point me in the direction of an FAQ/directions/YouTube video/book/anything for how to setup something like this? I suspect http://msdn.microsoft.com/en-us/library/hh850445.aspx#remote_3rd_party_connect_clone should get you started. As far as I can tell from #git, every once in a while people seem to want to escape from GUIs to have control over what they're doing (for example when cleaning up existing commits, or when performing a complex merge). You can find a usable commandline version of Git for Windows at http://msysgit.github.io/ (and the same project has a TortoiseSVN-like explorer plugin called GitCheetah if you need that). Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Harmful LESS flags
(cc-ing Mark Nudelman, less maintainer)
Hi,
d...@mailtor.net wrote:
Consider this diff, printed by `git diff`
#!/usr/bin/env python
-print('foo')
+print('bar')
Looks ok to merge and run.
But, after disabling the pager:
Unfortunately there are other kinds of subtle bugs that can be hard to
see in a terminal, too.
[...]
It would be nice if we could change the flags to either
a) avoid cutting off
b) indicate something has been cut off (- I prefer this)
That sounds like a nice feature request for 'less': a marker on the
right margin when --chop-long-lines is in use and a line has been
chopped. I don't see it at
http://www.greenwoodsoftware.com/less/bugs.html#enhance so maybe no
one else has thought of it yet.
Mark, what do you think?
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/9] test: add test_write_lines helper
Michael S. Tsirkin wrote:
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -712,6 +712,11 @@ test_ln_s_add () {
fi
}
+# This function writes out its parameters, one per line
+test_write_lines () {
+ printf %s\n $@;
+}
+
Thanks for fixing this.
Nits:
* no need for the trailing semicolon
* it's probably worth documenting this in t/README as well so people
writing new test scripts know what it's about.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 5/9] patch-id: document new behaviour
Michael S. Tsirkin wrote: Documentation/git-patch-id.txt | 23 ++- 1 file changed, 18 insertions(+), 5 deletions(-) Ah, there's the documentation. Please squash this with the patch that introduces the new behavior so they can be reviewed together more easily (both now and later when people do archeology). [...] +--stable:: + Use a symmetrical sum of hashes as the patch ID. + With this option, reordering file diffs that make up a patch or + splitting a diff up to multiple diffs that touch the same path + does not affect the ID. + This is the default if patchid.stable is set to true. This doesn't explain to me why I would want to use --stable versus --unstable. Maybe an EXAMPLES section would help? The only reason I can think of to use --unstable is for compatibility with historical patch-ids. Is there any other reason? At this point in the series there is no patchid.stable configuration. +--unstable:: + Use a non-symmetrical sum of hashes, such that reordering What is a non-symmetrical sum? Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/9] patch-id: make it stable against hunk reordering
Hi, Michael S. Tsirkin wrote: Patch id changes if users 1. reorder file diffs that make up a patch or 2. split a patch up to multiple diffs that touch the same path (keeping hunks within a single diff ordered to make patch valid). As the result is functionally equivalent, a different patch id is surprising to many users. Hm. If the goal is that functionally equivalent patches are guaranteed to produce the same patch-id, I wonder if we should be doing something like the following: 1. apply the patch in memory 2. generate a new diff 3. use that new diff to produce a patch-id Otherwise issues like --diff-algorithm=patience versus =myers will create trouble too. I don't think that avoiding false negatives for patch comparison without doing something like that is really possible. On the other hand if someone reorders file diffs within a patch, that is a potentially very common thing to do and something worth fixing. In other words, while your (1) makes perfect sense to me, case (2) seems less convincing. The downside of allowing reordering hunks is that it can potentially make different patches to be treated the same (for example if they were making similar changes to different functions) when the ordering previously caused them to be distinguished. But that wasn't something people could count on anyway, so I don't mind. Should the internal patch-id computation used by commands like 'git cherry' (see diff.c::diff_get_patch_id) get the same change? (Not a rhetorical question --- I don't know what the right choice would be there.) [...] The new behaviour is enabled - when patchid.stable is true - when --stable flag is present Using a new flag --unstable or setting patchid.stable to false force the historical behaviour. Which is the default? [...] builtin/patch-id.c | 89 -- 1 file changed, 73 insertions(+), 16 deletions(-) Documentation? Tests? Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What is missing from Git v2.0
Stefan Beller wrote: I may have missunderstood. So today you cannot commit if you don't provide an email address (usually the first time you try to commit, git asks to git config --global author.email=y...@mail.here), if I remember correctly, so there is definitely a valid (i.e. user approved) email address. Not true. But you do get a big wall of text when you make your first commit without an EMAIL envvar or configured [user] section, including | You can suppress this message by setting them explicitly: | | git config --global user.name Your Name | git config --global user.email y...@example.com | | After doing this, you may fix the identity used for this commit with: | | git commit --amend --reset-author Ciao, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Harmful LESS flags
Hi, David Kastrup wrote: Jeff King p...@peff.net writes: There are two questions here: 1. Can less do a better job of indicating what's in the input when -S is in effect? 2. What should get put into $LESS by default? I was specifically addressing (1). Your comment does not help at all there. It could have an impact on (2), but you didn't say anything besides I don't like it. That doesn't add anything to the conversation. No, I said it is useless, which is different from I don't like it. The information is not copypastable from a terminal window since it is cut off. It is also useless for review since one does not actually know what's in there. The only thing it has going for it is that it's prettier than the actually usable information. I disagree with your characterization of what's useful here, but it really doesn't matter. Why are you still arguing? I think it would be fine to change git's default for LESS to FRX and document that change wherever the documentation currently mentions FRSX, if someone wants to write a patch for it. (Such a change would sit in pu or next until after 2.0.0 is released, of course.) In the meantime, when you're on machines using the current default, you have two choices: a) set the LESS envvar in your .profile explicitly b) hit the two keys '-', shift+S when git opens a pager The argument about safety is a red herring here, since it's always possible that a patch will wrap to make new lines with '+' or '-' or '@@' at the beginning that are equally confusing. Hoping that clarifies, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Harmful LESS flags
Hi, Matthieu Moy wrote: I am personally in favor of changing the default to drop the S. Silently hiding stuff from the user's eyes is really bad. With good coding standard and reasonable terminal size, it actually doesn't matter. Just for clarity: no, when we are talking about well formatted code, -S is actually a way better interface. That's because indentation matters and makes it easy to take in code structure at a glance, long lines that get cut off by the margin stick out like a sore thumb already, and lines wrapped at an arbitrary character are even more distracting to the point of being useless. In practice I believe the Silently hiding stuff concern is much harder to solve. In the case of malicious code that opened this thread, I think a marker on the right margin would reveal the whitespace more clearly than wrapping that the reader may or may not notice. Luckily, it is very easy to switch between the two views on the fly --- in an already-open less window, you just type '-' + 'S'. In the spirit of not overriding tool defaults when there is not a strong reason to do so, I agree that if someone writes a patch to drop the 'S' I would probably like it. [...] I do not think we particularly need a transition plan here: it's purely a user-interface thing, not something that may break any script or other tool. Agreed a note in release notes and making sure the documentation reflects the new default should be enough. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What is missing from Git v2.0
Hi, David Kastrup wrote: Javier Domingo Cansino javier...@gmail.com writes: = Reject non-fast-forward pulls by default = Not having this introduced yet allows newbie people to use git with just 4 commands, without bothering around with fetch and merge and so. If you have a gun lying around your house, you should turn the safety catch off or the children will not be able to use it without instruction. I probably missed a subtlety, but the above comment reminded me of some netiquette I think this list is starting to forget. If I have misread it, please let me know and skip the rest of this message. This is a comment about style, not substance. Like coding style, discussion style matters as a way of making life easier for maintainers and new contributors, and different projects have subtly different practices. On the git list, the rule is simple. Feel free to grumble, but make sure there is some contribution in the same message. For example, after the confusing gun analogy you can say How about this patch? and people reading can follow up by reviewing that patch and potentially getting it applied. But what if there's already a patch doing what I want to do? you might ask. No problem. Link to the patch and say I think this patch should be revisited, or even better, re-send it with some notes about how previous review comments have been addressed or remain to be addressed. How do I get feedback on design of a new change without wasting a lot of time coding something that might be a bad idea? Glad you asked. Sometimes instead of a patch, a better contribution is a detailed explanation of a design to be reviewed and adopted. In this case, do try to be clear about whether you'll have enough time to implement the result or will be relying on others so people know how much time to devote to working out the details. What about feature requests? I might have an idea for improving git that no one's had before but I don't have a detailed design in mind. Sure, that can be a good contribution. Just treat it as a gift --- don't assume anyone is going to implement it for you if they're not interested. What about reminders about known bugs? There's this regression and it would not be right to release without fixing it but I think it's fallen through the cracks. Yes, good contribution. What about jokes? Sure, making people laugh is productive. What about cryptic grumbling? Sorry, unless you are grumbling to get input on how to improve git's documentation or code, it's not enough to be worth sending an email to this list. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Harmful LESS flags
David Kastrup wrote: Jonathan Nieder jrnie...@gmail.com writes: Just for clarity: no, when we are talking about well formatted code, -S is actually a way better interface. When we are talking about well-formatted code, -S does not matter either which way. Sorry for the lack of clarity. I believe well-formatted code can contain long lines. For example, sometimes a message + the printf to print it and indentation move past the right margin. If I wasn't talking about long lines, why would I have replied in the first place? [...] Overriding less' defaults should only be done for unequivocal benefits, We agree here. So, does someone who actually wants this change want to propose a patch? :) Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What is missing from Git v2.0
David Kastrup wrote: Jonathan Nieder jrnie...@gmail.com writes: I probably missed a subtlety, but the above comment reminded me of some netiquette I think this list is starting to forget. If I have misread it, please let me know and skip the rest of this message. [...] On the git list, the rule is simple. Feel free to grumble, but make sure there is some contribution in the same message. [...] Uh, Javier was commenting on a number of concrete proposals regarding the subject line What is missing from Git v2.0 and quoted Felipe directly. As you seem to have lost the context, let me requote the respective portion: I hadn't lost the context, but thanks for a pointer. [...] = Reject non-fast-forward pulls by default = [...] The patches were sent, the issues were addressed, people agreed, and yet nothing happened. [3][4] [...] [3] http://thread.gmane.org/gmane.comp.version-control.git/240030 [4] http://thread.gmane.org/gmane.comp.version-control.git/235981 Unfortunately Felipe's summary is incomplete. My message was meant as something that could be made into a reference for when similar questions of netiquette come up in the future (as for example they do all the time with Felipe). That meant I didn't give as good advice for your particular case than I should have. For this particular case, my thoughts are: If you believe those patches should be applied, please re-send them with a summary of changes you've made (if any) and your opinion on any unaddressed comments from the review thread. If you believe those patches should not be applied, wouldn't it be better to reply to that thread to help in case someone wants to pick up the patches and fix them? On the other hand if you just want to blow off steam, I guess that's fine too. Just don't expect it to result in any patches being applied, code quality improving, etc. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/11] refs.c: change ref_transaction_update() to do error checking and return status
Hi,
Ronnie Sahlberg wrote:
Update ref_transaction_update() do some basic error checking and return
true on error. Update all callers to check ref_transaction_update() for error.
Micronit: nonzero, not true. (true tends to mean '1' while here we
have the usual error return of -1. It's kind of annoying that C
doesn't have a nice way to distinguish between the usual int return of
0 for success and the usual bool return of true for success.)
Looks like a good change. Some tiny nitpicks below.
[...]
--- a/refs.h
+++ b/refs.h
@@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction
*transaction);
* that the reference should have had before the update, or zeros if
* it must not have existed beforehand.
*/
-void ref_transaction_update(struct ref_transaction *transaction,
+int ref_transaction_update(struct ref_transaction *transaction,
The comment above the prototype doesn't tell me:
When should the caller expect ref_transaction_update to return an
error? What does an error mean: is it always a sign of a bug in the
caller, or can it be due to some other problem? What guarantees does
the caller have about the state after an error --- is it just Things
will be in a sane state so you can free resources and exit, or will
the ref_transaction_update() have been essentially a no-op allowing
the caller to continue?
[...]
--- a/refs.c
+++ b/refs.c
@@ -3327,19 +3327,24 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
return update;
}
-void ref_transaction_update(struct ref_transaction *transaction,
+int ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
const unsigned char *new_sha1,
const unsigned char *old_sha1,
int flags, int have_old)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (have_old !old_sha1)
+ return error(have_old is true but old_sha1 is NULL);
I agree with Michael that the error message should start with BUG:
so humans encountering this know to contact the list instead of
blaming themselves.
Returning error instead of die-ing seems like a nice thing that make
it easier to make git valgrind-clean some day. Others might disagree
with me about whether that's worthwhile, but I think it's a good
change. :)
[...]
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -197,8 +197,10 @@ static const char *parse_cmd_update(struct strbuf
*input, const char *next)
if (*next != line_termination)
die(update %s: extra input: %s, refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+update_flags, have_old))
+ die(failed transaction update for %s, refname);
ref_transaction_update already printed an error, but of course that's
no guarantee that bugs in ref_transaction_update will not cause it
to fail without printing a message in the future. And the extra
context for the error might be nice (but why not print refname in
the message from ref_transaction_update instead?).
Is the plan for ref_transaction_update to be able to fail for
other reasons some day? What is the contract --- do we need a
human-readable, translatable message here, or is a this can't
happen BUG message fine?
I'd be fine with
die(BUG: failed transa...
or
/* ref_transaction_update already printed a message */
exit(128)
with a slight preference for the former, for what it's worth.
[...]
@@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(verify %s: extra input: %s, refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+update_flags, have_old))
+ die(failed transaction update for %s, refname);
Likewise.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 00/19] Use ref transactions from most callers
Ronnie Sahlberg wrote: This patch series changes most of the places where the ref functions for locking and writing refs to instead use the new ref transaction API. Thanks. Is this series based against mh/ref-transaction from next? [...] I think I have covered all issues raised on the previous reviews and also done a bunch of cleanups and improvements to the transaction API. Whoops, sorry I replied to an old message. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string
Ronnie Sahlberg wrote:
Let ref_transaction_commit return an optional error string that describes
the transaction failure. Start by returning the same error as update_ref_lock
returns, modulo the initial error:/fatal: preamble.
s/returns/prints/?
This will make it easier for callers to craft better error messages when
a transaction call fails.
Interesting. Can you give an example? What kind of behavior are we
expecting in callers other than die()-ing or cleaning up and then
die()-ing?
I like this more than having the caller pass in a flag/callback/etc to
decide how noisy to be and whether to gracefully accept errors or exit.
So it seems like an improvement, but may always returning error()
would be better --- more context would help in clarifying this.
--- a/refs.h
+++ b/refs.h
@@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem. The ref_transaction is freed by this function.
+ * If error is non-NULL it will return an error string that describes
+ * why a commit failed. This string must be free()ed by the caller.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
-const char *msg, enum action_on_err onerr);
+const char *msg, char **err,
+enum action_on_err onerr);
Is the idea that if I pass in a pointer err then
ref_transaction_commit will take the action described by onerr *and*
write its error message to err?
Probably squashing with patch 07 would make this easier to read (and
wouldn't require changing any messages at that point).
[...]
--- a/refs.c
+++ b/refs.c
[...]
@@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-flags,
update-type, onerr);
if (!update-lock) {
+ if (err) {
+ const char *str = Cannot lock the ref '%s'.;
+ *err = xmalloc(PATH_MAX + 24);
+ snprintf(*err, PATH_MAX + 24, str,
+ update-refname);
+ }
Might be clearer to use a helper similar to path.c::mkpathdup
char *aprintf(const char *fmt, ...)
{
char *result;
struct strbuf sb = STRBUF_INIT;
va_list args;
va_start(args, fmt);
strbuf_vaddf(sb, fmt, args);
va_end(args);
return strbuf_detach(sb);
}
or to have the caller pass in a pointer to strbuf instead of char *.
The rest looks good to me.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 04/19] refs.c: return error string from ref_update_reject_duplicates on failure
Ronnie Sahlberg wrote:
--- a/refs.c
+++ b/refs.c
@@ -3393,6 +3393,7 @@ static int ref_update_compare(const void *r1, const
void *r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+ char **err,
enum action_on_err onerr)
{
int i;
@@ -3400,6 +3401,11 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
if (!strcmp(updates[i - 1]-refname, updates[i]-refname)) {
const char *str =
Multiple updates for ref '%s' not allowed.;
+ if (err) {
+ *err = xmalloc(PATH_MAX + 41);
+ snprintf(*err, PATH_MAX + 41, str,
+ updates[i]-refname);
+ }
Same issues as the previous patch: it's too easy to get the buffer size
wrong when updating the message (or, worse, when making it
translatable). aprintf or a strbuf should work better.
Otherwise seems sensible.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 05/19] update-ref.c: use the error string from _commit to print better message
Ronnie Sahlberg wrote:
Call ref_transaction_commit with QUIET_ON_ERR and use the error string
that is returned to print a better log message if/after the transaction
fails.
Ah, so that's how the transition to a better API happens. Makes sense.
(A mention of QUIET_ON_ERR in the patch that introduces the err
parameter could help, or feel free to ignore these comments, since it's
all well by the end of the series.)
Update the tests to reflect that the log message is now slightly different
fatal: update_ref failed: Cannot lock the ref 'some ref'
versus from the previous
fatal: Cannot lock the ref 'some ref'
Makes sense as a demo of what the new code allows, but is this error
message better? The use of 'git update-ref' is an implementation
detail that the user doesn't need to know about.
I think I'd prefer the result of plain die(%s, err), even though
that's a no-op.
[...]
+++ b/builtin/update-ref.c
[...]
@@ -359,17 +360,18 @@ int cmd_update_ref(int argc, const char **argv, const
char *prefix)
die(Refusing to perform update with empty message.);
if (read_stdin) {
- int ret;
transaction = ref_transaction_begin();
-
+ if (!transaction)
+ die(failed to update refs);
This can't happen (xcalloc is defined to die() on malloc failure).
If you want to protect against it anyway, die(BUG: ...)?
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 06/19] refs.c: make update_ref_write to return error string on failure
Ronnie Sahlberg wrote: Change update_ref_write to also return an error string on failure. This makes the error avaialbel to ref_transaction_commit callers if the transaction failed dur to update_ref_sha1/write_ref_sha1 failures. Nits: * available * during Probably should come right after or before patch 3. Same nit as patch 3 about using asprintf or passing in a pointer to strbuf. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 07/19] refs.c: remove the onerr argument to ref_transaction_commit
Ronnie Sahlberg wrote: Since we now pass meaningful error messages back from ref_transaction_commit on failures, we no longer need to provide a onerr argument. Yay! More precisely, now that all callers use UPDATE_REFS_QUIET_ON_ERR there's no need to support any other behavior. Thanks for cleaning up the error handling here. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/12] MINGW: config.mak.uname: add explicit way to request MinGW-build
Hi, Marat Radchenko wrote: When crosscompiling, one cannot rely on `uname` from host system. Thus, add an option to use `make MINGW=1` for building MinGW build from non-MinGW host (Linux, for example). The same also applies when cross-compiling for any other platform, no? The consistent thing to do would be to add an ifdef block like this for every other platform,version,cpuarch triplet we care about, which doesn't seem like a great change. Can't the caller pass in uname_S=MINGW uname_M=x86_64 uname_O=MINGW uname_R= uname_P= uname_V= (or a single uname_A variable that the makefile could decompose if that is more convenient)? Of course that is fussy. It would be better to infer everything from CC, since the caller has to specify CC anyway. Does the output from $(CC) -dumpmachine have the information you need? Curious, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable
Hi, Marat Radchenko wrote: +# Define CROSS_COMPILE to specify the prefix used for all executables used +# during compilation. Only gcc and related bin-utils executables +# are prefixed with $(CROSS_COMPILE). Please include an example. # Define CROSS_COMPILE=foo- if your compiler and binary utilities # are foo-cc, foo-ar, foo-strip, etc. More specific variables # override this, so if you set CC=gcc CROSS_COMPILE=ia64-linux-gnu- # then the compiler will be 'gcc', not 'ia64-linux-gnu-gcc'. Otherwise unless I happen to know the convention from other packages I would not know whether to include a trailing '-' in CROSS_COMPILE, etc. Does the effect of this setting depend on whether CC=gcc (i.e., is the Makefile checking the value of CC and ignoring CROSS_COMPILE when it is e.g. the Intel compiler)? [...] -STRIP ?= strip +STRIP = $(CROSS_COMPILE)strip Before, STRIP from the environment took precedence over STRIP from the makefile. Switching to the more usual 'environment can't be trusted' convention is a good change, but please mention it in the commit message. The rest looks good from a quick look. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/12] MINGW: config.mak.uname: drop USE_NED_ALLOCATOR
Erik Faye-Lund wrote: On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko ma...@slonopotamus.org wrote: nedalloc was initially added in f0ed82 to fix slowness of standard WinXP memory allocator. Since WinXP is EOLed, this point is no longer valid. The actual reason behind this commit is incompatibility of malloc.c.h with MinGW-W64 headers. Alternative solution implies updating nedalloc to something newer. Did you measure that malloc on newer Windows-versions are actually faster? AFAIK, malloc does a lot more inside the CRT than in the kernel... If it does turn out that nedalloc is not needed any more, please remove it from compat/, too. ;-) Thanks for cleaning up. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] Makefile: use curl-config to determine curl flags
Hi, Dave Borowitz wrote: curl-config is usually installed alongside a curl distribution, and its purpose is to provide flags for building against libcurl, so use it instead of guessing flags and dependent libraries. The previous version of these two patches is already part of master. Could you make an incremental patch? Sorry for the fuss, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Reference to a commit inside a commit message
Hi, enzodici...@gmail.com wrote: Hi to all, I'm trying to figure out what is the best way (and if it exists) to link a message of a commit to another commit. [...] Obviously I don't mean to put the raw Hash, Why not? See the output of git log --grep='In commit ' and git log --grep='In v' in git.git or linux.git for some examples of current practice. Curious, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
Dave Borowitz wrote: On Mon, Apr 28, 2014 at 1:05 PM, Jonathan Nieder jrnie...@gmail.com wrote: Dave Borowitz wrote: +++ b/Makefile @@ -35,7 +35,9 @@ all:: # transports (neither smart nor dumb). # # Define CURL_CONFIG to the path to a curl-config binary other than the -# default 'curl-config'. +# default 'curl-config'. If CURL_CONFIG is unset or points to a binary that +# is not found, defaults to the CURLDIR behavior, or if CURLDIR is not set, +# uses -lcurl with no additional library detection. I'm having a little trouble parsing this but don't have any better suggestion. How about: If CURL_CONFIG is unset or points to a binary that is not found, defaults to the CURLDIR behavior. If CURLDIR is not set, this means using -lcurl with no additional library detection (other than NEEDS_*_WITH_CURL). Yep, that's clearer. [...] - $(error libcurl not detected; try setting CURLDIR) +$(error libcurl not detected or not compiled with static support) Whitespace damage. Yes, but intentional, because Makefile parsing is weird. $ echo -e 'ifndef FOO\n\t$(error bad things)\nendif\n\nfoo:\n\ttouch foo' mk1 make -f mk1 foo mk1:2: *** commands commence before first target. Stop. $ echo -e 'ifndef FOO\n $(error bad things)\nendif\n\nfoo:\n\ttouch foo' mk2 make -f mk2 foo mk2:2: *** bad things. Stop. Gah. Maybe it should be left-justified to avoid accentally breaking it again. Thanks. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable
Hi, Marat Radchenko wrote: On Mon, Apr 28, 2014 at 09:25:36AM -0700, Jonathan Nieder wrote: -STRIP ?= strip +STRIP = $(CROSS_COMPILE)strip Before, STRIP from the environment took precedence over STRIP from the makefile. Switching to the more usual 'environment can't be trusted' convention is a good change, but please mention it in the commit message. Taken from [1]: Simply expanded variables are defined by I'm not really sure what in particular you're pointing to in that page. If you have a more specific question about what '?=' means, could you say it? Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
Dave Borowitz wrote: Signed-off-by: Dave Borowitz dborow...@google.com --- Makefile | 41 - 1 file changed, 28 insertions(+), 13 deletions(-) For what it's worth, Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks for the quick turnaround. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable
Marat Radchenko wrote: On Mon, Apr 28, 2014 at 12:37:42PM -0500, Felipe Contreras wrote: +CC = $(CROSS_COMPILE)cc Nice. Actually, not. You still have to override CC because it is $(CROSS_COMPILE)*g*cc. Any thoughts how to handle this? One possibility would be something like ifdef CROSS_COMPILE CC = $(CROSS_COMPILE)gcc else CC = cc endif Or as Felipe says, you can try to lobby your distro to install the symlink. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pull is Evil
Marc Branchaud wrote: All that said, I don't object to any attempts at improving the command either. But I also don't see any kind of improvement that would lead me to start using git pull let alone recommending it to new users. If git pull starts using --ff-only by default then I might start recommending it. I'm a little scared to look at the details of this thread. Hopefully once the topic matures and settles down a little it will be worthwhile to review, or if there's any way I can help before then, feel free to ask me privately. Thanks for your work, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Problem: staging of an alternative repository
Hi Pavel, Pasha Bolokhov wrote: It turns out Git treats the directory '.git' differently enough from everything else. That may be ok, Yeah, it's intended. [...] if you supply a different repository base name, say, '.git_new', by either setting GIT_DIR or using the '--git-dir' option, Git 'add' will not make any exception for it and think of it as a new (weird) directory. Yep, a git repository metadata directory named .git_new is not special in any way and you can use git add to track it if you want (for example to add a testcase). [...] Now I know, the '--git-dir' option may usually be meant to use when the repository is somewhere outside of the work tree, and such a problem would not arise. And even if it is inside, sure enough, you can add this '.git_new' to the ignores or excludes. But is this really what you expect? I think it's more that it never came up. Excluding the current $GIT_DIR from what git add can add (on top of the current rule of excluding all instances of .git) seems like a sensible change, assuming it can be done without hurting the code too much. ;-) But as you note, you are not using $GIT_DIR the way it was intended to be used. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG?] Patches created with 'diff.noprefix=true' don't 'git apply'.
Hi, Nathan Collins wrote: Patches created with 'diff.noprefix=true' don't 'git apply' without specifying '-p0'. I'm not sure this is a bug -- the 'man git-apply' just says Reads the supplied diff output (i.e. a patch) and applies it to files -- but I would expect patches I create locally to apply cleanly locally. Sounds like a documentation bug, at least. Any ideas for clearer wording? In real life the 'diff.noprefix=true' is in my ~/.gitconfig, so this was pretty confusing. I personally think setting diff.noprefix is not very sane (it also breaks patch -p1), and I suppose I should have been louder about that when it was introduced. Can you say more about the workflow you use that requires diff.noprefix? Maybe we can make other changes to improve it, too. At first glance I don't suspect making diff.noprefix imply -p0 for git am would be great, since that would generate the the opposite problem when applying patches from the outside world. But maybe we need better autodetection and maybe noprefix is a good signal about when to use it. Another complication is that unlike 'git diff', 'git apply' is plumbing that is meant to be useful and reliable for scripts. And unlike most plumbing, there is no higher-level command with similar functionality for which we can experiment more freely with the UI. Adding a new command to fix that might be a good direction toward handling noprefix patches better. [...] git show | git apply --reverse The following which only uses plumbing commands should work: git diff-tree -p HEAD^! | git apply --reverse Thanks for some food for thought, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Define constants for lengths of object names
Hi,
brian m. carlson wrote:
--- a/object.h
+++ b/object.h
[...]
@@ -49,7 +56,7 @@ struct object {
unsigned used : 1;
unsigned type : TYPE_BITS;
unsigned flags : FLAG_BITS;
- unsigned char sha1[20];
+ unsigned char sha1[GIT_OID_RAWSZ];
Maybe my brain has been damaged by reading code from too many C
projects that hard-code some constants, but I find '20' easier to read
here.
What happened to the
struct object_id {
unsigned char id[20];
};
...
struct object {
...
struct object_id id;
};
idea?
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Define constants for lengths of object names
Jonathan Nieder wrote:
brian m. carlson wrote:
--- a/object.h
+++ b/object.h
[...]
@@ -49,7 +56,7 @@ struct object {
unsigned used : 1;
unsigned type : TYPE_BITS;
unsigned flags : FLAG_BITS;
-unsigned char sha1[20];
+unsigned char sha1[GIT_OID_RAWSZ];
Maybe my brain has been damaged by reading code from too many C
projects that hard-code some constants, but I find '20' easier to read
here.
That said, RAW_SHA1_BYTES would sound okay to me.
Part of the problem was how long it takes to mentally parse oid_rawsz.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git send-email doesn't work with IPv6 and STARTTLS
Hi, Steffen Ullrich wrote: git-send-mail does not use Net::SMTP directly for SSL support, but: - for direct connections (port 465) it uses Net::SMTP::SSL which just replaces the superclass if Net::SMTP with IO::Socket::SSL and thus implicitly supports IPv6 (because IO::Socket::SSL does) - for plain connections with SSL upgrade git-send-mail uses Net::SMTP for the initial connect and then does Net::SMTP::SSL-start_SSL (e.g. inherited from IO::Socket::SSL) to upgrade the socket to SSL. The problem here is that Net::SMTP does not support IPv6, but this should be solvable by using Net::INETGlue::INET_is_INET6 before loading Net::SMTP. Sounds like a good change, fwiw. Even after Net::SMTP is fixed, it would be useful as a way to make sure git works well with old versions of Net::SMTP too. But all these tricks are just workarounds for missing IPv6 and SSL support directly in the Net::SMTP, Net::FTP and Net::POP3. I therefore repeat my proposal from RT#93823 (no response yet) to add transparent support for IPv6 and SSL into these modules. By transparent I mean that the features are available if the necessary modules are installed (e.g. IO::Socket::SSL for SSL and IO::Socket::INET6 or IO::Socket::IP for IPv6), but that it works like before if they are not installed. I don't have these patches yet, but most of the necessary code is already there in Net::SSLGlue and Net::INET6Glue. Would you accept and incorporate such patches? Thanks for a kind offer. I am not Net::SMTP upstream, but I'd be happy to work with upstream to get such patches applied. Thanks for your work, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Zsh submodule name completion borked
Hi, Phil Hord wrote: When I use zsh tab-completion to complete the submodule name in 'git submodule init', I get more than I expected. Is this using zsh's native tab-completion (i.e., not the tab completion bundled with git)? There might have been a change there. Another place to look for hints would be ~/.gitconfig. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Define constants for lengths of object names
brian m. carlson wrote:
On Thu, May 01, 2014 at 10:20:07AM -0700, Jonathan Nieder wrote:
What happened to the
struct object_id {
unsigned char id[20];
};
...
struct object {
...
struct object_id id;
};
idea?
There didn't seem to be a huge amount of support for it.
I can make up for it in enthuasiasm. Please? It's something I've
wanted for a long time but never found the time to do.
Also, there
were concerns that some architectures might impose alignment constraints
on it that made sizeof(struct object_id) != 20.
Sounds awful. What architecture?
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: #178 parsing of pretty=format:%an %ad causes fatal: bad revision '%ad'
Hi Dave, Dave Bradley wrote: G:\ws_test_env\GIT_TESTBED_TMP\fest-swing-1.xgit log --all --pretty=format:%an %ad -- pom.xml Mon Nov 23 03:09:17 2009 + Mon Nov 23 02:42:24 2009 + G:\ws_test_env\GIT_TESTBED_TMP\fest-swing-1.xgit log --all --pretty=format:%an %ad -- pom.xml fatal: bad revision '%ad' On Linux, this example gets passed to git as six arguments: log --all --pretty=format:%an %ad -- pom.xml I think the intent was instead to pass five arguments (the third being '--pretty=format:%an %ad'). That means you shouldn't unquote before the space, or in other words that the space should be part of a quoted argument. On Windows, I believe the argument passing convention is more complicated. Programs can inspect the entire command line if they want to. But there's still an ambiguity in the command you passed: if I look at space-separated or double-quoted parts of the command line, it looks like git log --all --pretty=format: (no space) %an %ad (no space) -- pom.xml What's the right way to parse this? How can git tell whether %an %ad were meant to be separate arguments or not? In absence of a stronger convention I suspect the simplest rule is to mimic what a Unix shell does, where they are separate arguments because the space is not quoted. Cc-ing Windows folks in case they have more insight. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: #178 parsing of pretty=format:%an %ad causes fatal: bad revision '%ad'
(resending with the correct address for the Git for Windows developers. Sorry for the noise.) Hi Dave, Dave Bradley wrote: G:\ws_test_env\GIT_TESTBED_TMP\fest-swing-1.xgit log --all --pretty=format:%an %ad -- pom.xml Mon Nov 23 03:09:17 2009 + Mon Nov 23 02:42:24 2009 + G:\ws_test_env\GIT_TESTBED_TMP\fest-swing-1.xgit log --all --pretty=format:%an %ad -- pom.xml fatal: bad revision '%ad' On Linux, this example gets passed to git as six arguments: log --all --pretty=format:%an %ad -- pom.xml I think the intent was instead to pass five arguments (the third being '--pretty=format:%an %ad'). That means you shouldn't unquote before the space, or in other words that the space should be part of a quoted argument. On Windows, I believe the argument passing convention is more complicated. Programs can inspect the entire command line if they want to. But there's still an ambiguity in the command you passed: if I look at space-separated or double-quoted parts of the command line, it looks like git log --all --pretty=format: (no space) %an %ad (no space) -- pom.xml What's the right way to parse this? How can git tell whether %an %ad were meant to be separate arguments or not? In absence of a stronger convention I suspect the simplest rule is to mimic what a Unix shell does, where they are separate arguments because the space is not quoted. Cc-ing Windows folks in case they have more insight. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG or FEATURE? Use of '/' in branch names
Hi Keith, Keith Derrick wrote: $ git checkout -b hotfix Switched to a new branch 'hotfix' $ git checkout -b hotfix/b2 error: unable to resolve reference refs/heads/hotfix/b2: Not a directory fatal: Failed to lock ref for update: Not a directory $ That's an ugly message. I think we can do better. (hint hint) Longer term, I think people would like to make it possible for a 'hotfix' and 'hotfix/b2' branch to coexist, but that will take some work, and until then, git tries to be careful about enforcing the constraint that they cannot coexist. Fixing it would be complicated by the need to avoid breaking people with older versions of git when they fetch from you (what happens to the origin/hotfix and origin/hotfix/b2 remote-tracking refs on the client side?). Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG or FEATURE? Use of '/' in branch names
Hi, Keith Derrick wrote: Yes, I've since found some discussion on this, and had already changed to use '-' to append the classifier. But the other problem is that I can't easily find this restriction documented anywhere - which means it comes as a suprise to people. That sounds like another serious bug (the first one was the lousy error message). The current behavior is intended, and it sounds like the documentation is lagging. Where did you expect to find information about this? Knowing that would help a lot in fixing it. (The nicest way to indicate where you expected to read about this is with a patch against the relevant file in Documentation/*.txt, of course.) Thanks again, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pull is Mostly Evil
Hi, Philip Oakley wrote: That assumes that [git pull] doing something is better than doing nothing, which is appropriate when the costs on either side are roughly similar. I think the conversation's going around in circles. Potential next steps: a. Documentation or test patch illustrating desired behavior b. More traditional formal design doc explaining desired behavior and the thinking behind it (problem, overview of solution, alternatives rejected, complications, example, open questions). c. Implementation patch d. Someone takes an existing patch and figures out the next step toward getting it ready for application. My preference is for (a), I guess. The point being that something more concrete (code or a design doc) makes it easier to avoid talking past each other. And having something concrete to edit makes the stakes clearer so people can make it incrementally better without being distracted by unimportant parts. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pager: remove 'S' from $LESS by default
Hi, Matthieu Moy wrote: By default, Git used to set $LESS to -FRSX if $LESS was not set by the user. The FRX flags actually make sense for Git (F and X because Git sometimes pipes short output to less, and R because Git pipes colored output). The S flag (chop long lines), on the other hand, is not related to Git and is a matter of user preference. Git should not decide for the user to change LESS's default. Thanks! Sounds like a very good change. (Nit: instead of because Git sometimes pipes short output to less, it would be clearer to say something like when Git pipes short output to less it is nice to exit and let the user type their next command.) [...] The documentation in config.txt is made a bit longer to keep both an example setting the 'S' flag (needed to recover the old behavior) and an example showing how to unset a flag set by Git. Interesting. Looks good. For what it's worth, Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG?] Patches created with 'diff.noprefix=true' don't 'git apply'.
Nathan Collins wrote: On Wed, Apr 30, 2014 at 7:40 PM, Jonathan Nieder jrnie...@gmail.com wrote: Nathan Collins wrote: git show | git apply --reverse The following which only uses plumbing commands should work: git diff-tree -p HEAD^! | git apply --reverse Nice! However, I don't now how to generalize this solution to other (probably insane) use cases, e.g. git log -Sstring --patch | git apply --reverse This should do it: git rev-list HEAD | git diff-tree --no-commit-id -p -Sstring --stdin | git apply --reverse More generally, when scripting plumbing commands tend to do the right thing. Will think more about the documentation and other parts (or if someone else sends a patch before I can, I won't mind). Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] merge-recursive.c: Fix case-changing merge.
Junio C Hamano wrote: dtur...@twopensource.com writes: +test -e testcase Please make it a habit to use test -f when you expect the path exists as a file, not merely something exists there I do not care if it is a file or a directory, for which test -e is perfectly appropriate. Or, better in tests, test_path_is_file testcase which prints an error instead of just silently failing when the path is not a file. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] shell doc: remove stray + in example
The git-shell(1) manpage says EXAMPLE To disable interactive logins, displaying a greeting instead: + $ chsh -s /usr/bin/git-shell $ mkdir $HOME/git-shell-commands [...] The stray + has been there ever since the example was added in v1.8.3-rc0~210^2 (shell: new no-interactive-login command to print a custom message, 2013-03-09). The + sign between paragraphs is needed in asciidoc to attach extra paragraphs to a list item but here it is not needed and ends up rendered as a literal +. Remove it. A quick search with grep -e 'p+' /usr/share/doc/git/html/*.html doesn't find any other instances of this problem. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Documentation/git-shell.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt index c35051b..e4bdd22 100644 --- a/Documentation/git-shell.txt +++ b/Documentation/git-shell.txt @@ -66,7 +66,7 @@ EXAMPLE --- To disable interactive logins, displaying a greeting instead: -+ + $ chsh -s /usr/bin/git-shell $ mkdir $HOME/git-shell-commands -- 1.9.1.423.g4596e3a -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Submodule improvements (Re: What's cooking in git.git (Apr 2014, #09; Tue, 29))
Hi, David Lang wrote: I haven't been paying close attention for a while, what would have to be done to make submodules an integral part of Git? The series at http://thread.gmane.org/gmane.comp.version-control.git/241455 is a start. I'm hoping to get a reroll done soon and then I can talk about later steps. https://github.com/jlehmann/git-submod-enhancements/wiki has a rough roadmap, but really there's lots of commands that could be improved to recurse into submodules and not many interdependencies involved so anyone can bite off a chunk. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Conforming to pep8
(cc-ing Pete Wyckoff who maintains git-p4 and Michael Haggerty
who maintains git-multimail)
Hi,
William Giokas wrote:
- We follow PEP-8 (http://www.python.org/dev/peps/pep-0008/).
It's even the first thing that you see when you go looking for 'python'
in the coding style document. I just ran every file in the tree that
either ended in '.py' or had a python #!, and was greeted with a whole
bunch of output::
./git-p4.py
./contrib/svn-fe/svnrdump_sim.py
./contrib/remote-helpers/git-remote-bzr
./contrib/hooks/multimail/post-receive
./contrib/hooks/multimail/migrate-mailhook-config
./contrib/hooks/multimail/git_multimail.py
./contrib/hooks/multimail/README
./contrib/hg-to-git/hg-to-git.py
./contrib/gitview/gitview
./contrib/fast-import/import-zips.py
Thanks for running this check. Passing on the result to the
maintainers of some of those scripts in case they have thoughts.
As someone involved in contrib/svn-fe/, I would be happy to take a
patch making svnrdump_sim.py follow PEP-8, if you have time to write
one.
Thanks,
Jonathan
List of warnings kept below for reference.
20 E101 indentation contains mixed spaces and tabs
90 E111 indentation is not a multiple of four
9 E112 expected an indented block
47 E113 unexpected indentation
1 E121 continuation line under-indented for hanging indent
3 E122 continuation line missing indentation or outdented
3 E124 closing bracket does not match visual indentation
12 E125 continuation line with same indent as next logical line
9 E126 continuation line over-indented for hanging indent
4 E127 continuation line over-indented for visual indent
63 E128 continuation line under-indented for visual indent
4 E129 visually indented line with same indent as next logical line
3 E131 continuation line unaligned for hanging indent
37 E201 whitespace after '['
30 E202 whitespace before ']'
30 E203 whitespace before ':'
37 E211 whitespace before '('
10 E221 multiple spaces before operator
14 E222 multiple spaces after operator
8 E223 tab before operator
1 E224 tab after operator
35 E225 missing whitespace around operator
6 E228 missing whitespace around modulo operator
23 E231 missing whitespace after ','
10 E251 unexpected spaces around keyword / parameter equals
1 E261 at least two spaces before inline comment
1 E262 inline comment should start with '# '
37 E265 block comment should start with '# '
1 E301 expected 1 blank line, found 0
117 E302 expected 2 blank lines, found 1
19 E303 too many blank lines (2)
4 E401 multiple imports on one line
220 E501 line too long (83 79 characters)
5 E502 the backslash is redundant between brackets
33 E701 multiple statements on one line (colon)
11 E702 multiple statements on one line (semicolon)
34 E703 statement ends with a semicolon
9 E711 comparison to None should be 'if cond is None:'
2 E713 test for membership should be 'not in'
1022W191 indentation contains tabs
40 W601 .has_key() is deprecated, use 'in'
1 W602 deprecated form of raising exception
1 W603 '' is deprecated, use '!='
1 W604 backticks are deprecated, use 'repr()'
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 12/25] contrib: remove 'vim'
Hi, Jeff King wrote: However, I would certainly agree that that period of time is probably over; the scripts started shipping in upstream vim in mid-2008. I'd be happy to see this directory go away whether or not the rest of contrib/ is dropped. RHEL 6 has vim 7.2.something, so yeah, this should be mostly safe. Git needs to keep working for people stuck on RHEL 5, but niceties like vim support seem less important there. But I am not convinced it is worth inconveniencing them (or putting any obstacles in the way of upgrading) without a good reason, and one less directory in contrib/ does not seem like a very strong reason. Here's a new commit message in case we want to do this. -- 8 -- Subject: contrib: remove vim support instructions The git support scripts started shipping in upstream vim in version 7.2 (2008-08-09). Clean up contrib/ a little by removing the instructions for people on older versions of vim. RHEL 6 already has vim 7.2.something, so anyone on a reasonably modern operating system should not be affected. Users on RHEL 5 presumably know that means sometimes missing out on niceties like syntax highlighting, so this should be safe. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- contrib/vim/README | 22 -- 1 file changed, 22 deletions(-) delete mode 100644 contrib/vim/README diff --git a/contrib/vim/README b/contrib/vim/README deleted file mode 100644 index 8f16d06..000 --- a/contrib/vim/README +++ /dev/null @@ -1,22 +0,0 @@ -Syntax highlighting for git commit messages, config files, etc. is -included with the vim distribution as of vim 7.2, and should work -automatically. - -If you have an older version of vim, you can get the latest syntax -files from the vim project: - - http://ftp.vim.org/pub/vim/runtime/syntax/git.vim - http://ftp.vim.org/pub/vim/runtime/syntax/gitcommit.vim - http://ftp.vim.org/pub/vim/runtime/syntax/gitconfig.vim - http://ftp.vim.org/pub/vim/runtime/syntax/gitrebase.vim - http://ftp.vim.org/pub/vim/runtime/syntax/gitsendemail.vim - -These files are also available via FTP at the same location. - -To install: - - 1. Copy these files to vim's syntax directory $HOME/.vim/syntax - 2. To auto-detect the editing of various git-related filetypes: - - $ curl http://ftp.vim.org/pub/vim/runtime/filetype.vim | - sed -ne '/^ Git$/, /^$/ p' $HOME/.vim/filetype.vim -- 1.9.1.423.g4596e3a -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] svn-fe: Conform to pep8
Hi, William Giokas wrote: Quite a large change, most of this was whitespace changes, though there were a few places where I removed a comma or added a few characters. Should pass through pep8 and pass every test. Thanks! Mind if I forge your sign-off? (See Documentation/SubmittingPatches section (5) Sign your work for what this means.) Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 04/25] contrib: remove 'buildsystems'
Hi, Erik Faye-Lund wrote: On Fri, May 9, 2014 at 10:14 AM, Felipe Contreras felipe.contre...@gmail.com wrote: Erik Faye-Lund wrote: Please don't. This script is useful to build with the MSVC IDE, which enables us to use their excellent debugger. If you want this script to remain in contrib, please: a) Write at least a few tests b) Write some documentation c) Explain why it cannot live outside the git.git repository like other tools. [1][2][3] (Adding Marius, the original author to the CC-list) Uh, why is such a burden required all of a sudden? It isn't. As far as I can tell, the only point of this patch series was to prove a point. If a patch from the series happens to be useful then that's great, but otherwise it's mostly an act of protest as far as I can tell. (From my point of view, this kind of protest is not really a good way to reward people who have made good contributions to contrib/ in the past, so I hope it doesn't happen often.) I'm happy the MSVC build scripts are in git.git. Thanks for keeping them maintained well. Sincerely, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 06/25] contrib: remove 'diffall'
Hi Tim, Tim Henigan wrote: On Thu, May 8, 2014 at 5:58 PM, Felipe Contreras felipe.contre...@gmail.com wrote: contrib/diffall/README | 31 -- contrib/diffall/git-diffall | 257 2 files changed, 288 deletions(-) delete mode 100644 contrib/diffall/README delete mode 100755 contrib/diffall/git-diffall I see no problem with removing this script from contrib. However, the commit message should mention that git-difftool learned all the features of git-diffall when the '--dir-diff' option was added in v1.7.11 (ca. June 2012). A few questions: * Do you still use git-diffall? Since it hasn't been a maintenance burden, I wouldn't mind keeping it if it still has users. * Any thoughts about how to help people who have been using it to migrate to difftool? Would a note in the release notes to look into the --dir-diff option to difftool be enough, or are there more specific pointers that could be useful? Once those questions are dealt with, this seems like a nice small cleanup. Thanks for the quick feedback. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 06/25] contrib: remove 'diffall'
Hi,
Tim Henigan wrote:
However, I like the idea of simply removing the directory from contrib
and pointing to 'difftool --dir-diff' in the release notes.
Thanks. I believe Junio uses commit messages as reference when
writing release notes so hopefully this should be enough to make that
happen.
-- 8 --
Subject: contrib: remove git-diffall
The functionality of the git diffall script in contrib/ was
incorporated into git difftool when the --dir-diff option was added
in v1.7.11 (ca. June, 2012). Once difftool learned those features,
the diffall script became obsolete.
The only difference in behavior is that when comparing to the working
tree, difftool copies any files modified by the user back to the
working tree when the diff tool exits. git diffall required the
--copy-back option to do the same. All other diffall options have the
same meaning in difftool.
Make life easier for people choosing a tool to use by removing the old
diffall script. A pointer in the release notes should be enough to
help current users migrate.
Helped-by: Tim Henigan tim.heni...@gmail.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
contrib/diffall/README | 31 --
contrib/diffall/git-diffall | 257
2 files changed, 288 deletions(-)
delete mode 100644 contrib/diffall/README
delete mode 100755 contrib/diffall/git-diffall
diff --git a/contrib/diffall/README b/contrib/diffall/README
deleted file mode 100644
index 507f17d..000
--- a/contrib/diffall/README
+++ /dev/null
@@ -1,31 +0,0 @@
-The git-diffall script provides a directory based diff mechanism
-for git.
-
-To determine what diff viewer is used, the script requires either
-the 'diff.tool' or 'merge.tool' configuration option to be set.
-
-This script is compatible with most common forms used to specify a
-range of revisions to diff:
-
- 1. git diffall: shows diff between working tree and staged changes
- 2. git diffall --cached [commit]: shows diff between staged
- changes and HEAD (or other named commit)
- 3. git diffall commit: shows diff between working tree and named
- commit
- 4. git diffall commit commit: show diff between two named commits
- 5. git diffall commit..commit: same as above
- 6. git diffall commit...commit: show the changes on the branch
- containing and up to the second, starting at a common ancestor
- of both commit
-
-Note: all forms take an optional path limiter [-- path*]
-
-The '--extcmd=command' option allows the user to specify a custom
-command for viewing diffs. When given, configured defaults are
-ignored and the script runs $command $LOCAL $REMOTE. Additionally,
-$BASE is set in the environment.
-
-This script is based on an example provided by Thomas Rast on the
-Git list [1]:
-
-[1] http://thread.gmane.org/gmane.comp.version-control.git/124807
diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
deleted file mode 100755
index 84f2b65..000
--- a/contrib/diffall/git-diffall
+++ /dev/null
@@ -1,257 +0,0 @@
-#!/bin/sh
-# Copyright 2010 - 2012, Tim Henigan tim.heni...@gmail.com
-#
-# Perform a directory diff between commits in the repository using
-# the external diff or merge tool specified in the user's config.
-
-USAGE='[--cached] [--copy-back] [-x|--extcmd=command] commit{0,2} [--
path*]
-
---cached Compare to the index rather than the working tree.
-
---copy-back Copy files back to the working tree when the diff
- tool exits (in case they were modified by the
- user). This option is only valid if the diff
- compared with the working tree.
-
--x=command
---extcmd=command Specify a custom command for viewing diffs.
- git-diffall ignores the configured defaults and
- runs $command $LOCAL $REMOTE when this option is
- specified. Additionally, $BASE is set in the
- environment.
-'
-
-SUBDIRECTORY_OK=1
-. $(git --exec-path)/git-sh-setup
-
-TOOL_MODE=diff
-. $(git --exec-path)/git-mergetool--lib
-
-merge_tool=$(get_merge_tool)
-if test -z $merge_tool
-then
- echo Error: Either the 'diff.tool' or 'merge.tool' option must be set.
- usage
-fi
-
-start_dir=$(pwd)
-
-# All the file paths returned by the diff command are relative to the root
-# of the working copy. So if the script is called from a subdirectory, it
-# must switch to the root of working copy before trying to use those paths.
-cdup=$(git rev-parse --show-cdup)
-cd $cdup || {
- echo 2 Cannot chdir to $cdup, the toplevel of the working tree
- exit 1
-}
-
-# set up temp dir
-tmp=$(perl -e 'use File::Temp qw(tempdir);
- $t=tempdir(/tmp/git-diffall.X) or exit(1);
- print $t') || exit 1
-trap 'rm -rf $tmp' EXIT
-
-left=
-right=
-paths=
-dashdash_seen=
-compare_staged=
-merge_base=
-left_dir=
-right_dir=
-diff_tool=
-copy_back=
-
-while test $# != 0
-do
- case $1 in
- -h|--h|--he|--hel
Re: [PATCH 1/2] inline constant return from error() function
Hi,
Jeff King wrote:
On Mon, May 05, 2014 at 05:29:38PM -0400, Jeff King wrote:
I cannot think of any other way to make the compiler aware of the
constant value, but perhaps somebody else is more clever than I am.
This came to me in a dream, and seems to work.
Clever. :) Thanks for thinking it up.
[...]
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -331,7 +331,11 @@ extern void warning(const char *err, ...)
__attribute__((format (printf, 1, 2)))
* using the function as usual.
*/
#if defined(__GNUC__) ! defined(__clang__)
-#define error(...) (error(__VA_ARGS__), -1)
+static inline int const_error(void)
+{
+ return -1;
+}
+#define error(...) (error(__VA_ARGS__), const_error())
I wish we could just make error() an inline function that calls some
print_error() helper, but I don't believe all compilers used to build
git are smart enough to inline uses of varargs. So this looks like
the right thing to do.
I kind of wish we weren't in so much of an arms race with static
analyzers. Is there some standard way to submit our code as an idiom
that should continue not to produce warnings to be included in a
testsuite and prevent problems in the future?
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git format-patch --signature string | file
Hi, Jeremiah Mahler wrote: # from a string $ git format-patch --signature from a string origin # or from a file $ git format-patch --signature ~/.signature origin Interesting. But... what if I want my patch to end with -- /home/jrnieder/.signature ? It seems safer to introduce a separate --signature-file option. [...] builtin/log.c | 26 -- 1 file changed, 24 insertions(+), 2 deletions(-) Tests? Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 00/42] Use ref transactions for all ref updates
Hi, Ronnie Sahlberg wrote: This patch series is based on next and expands on the transaction API. Sorry to take so long to get to this. For the future, it's easier to review patches based on some particular branch that got merged into next, since next is a moving target (series come and go from there depending on what seems to need testing at a given moment). Is mh/ref-transaction the relevant branch to build on? Trying to apply the series to mh/ref-transaction, I get conflicts in patch 13 due to absence of rs/ref-update-check-errors-early. Trying to apply the series to a merge of mh/ref-transaction and rs/ref-update-check-errors-early, I get a minor conflict in patch 15 but it is easy to resolve and the rest goes smoothly. Looking forward to reading the rest. Thanks. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 02/42] refs.c: allow passing NULL to ref_transaction_free
Ronnie Sahlberg wrote:
Allow ref_transaction_free to be called with NULL and in extension allow
ref_transaction_rollback to be called for a NULL transaction.
In extension = as a result?
Makes sense. It lets someone do the usual
struct ref_transaction *transaction;
int ret = 0;
if (something_fails()) {
ret = -1;
goto cleanup;
}
...
cleanup:
ref_transaction_free(transaction);
return ret;
just like you can already do with free().
This allows us to write code that will
if ( (!transaction ||
ref_transaction_update(...)) ||
(ref_transaction_commit(...) !(transaction = NULL)) {
ref_transaction_rollback(transaction);
...
}
Somewhere in the whitespace and parentheses I'm lost.
Is the idea that when ref_transaction_commit fails it will have
freed the transaction so we need not to roll back to prevent a
double free? I think it would be simpler for the caller to
unconditionally set transaction to NULL after calling
ref_transaction_commit in such a case to avoid use-after-free.
Even better if it is the caller's responsibility to free
the transaction. At any rate, it doesn't seem related to this
patch.
--- a/refs.c
+++ b/refs.c
@@ -3303,6 +3303,9 @@ static void ref_transaction_free(struct ref_transaction
*transaction)
{
int i;
+ if (!transaction)
+ return;
Except for the unclear commit message,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 03/42] refs.c: add a strbuf argument to ref_transaction_commit for error logging
Ronnie Sahlberg wrote:
Add a strbuf argument to _commit so that we can pass an error string back to
the caller. So that we can do error logging from the caller instead of from
_commit.
Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
and craft any log messages from the callers themselves and finally remove the
onerr argument completely.
Very nice.
[...]
+++ b/refs.c
[...]
@@ -3443,6 +3444,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-flags,
update-type, onerr);
if (!update-lock) {
+ if (err)
+ strbuf_addf(err ,Cannot lock the ref '%s'.,
+ update-refname);
Tiny nit: whitespace.
[...]
--- a/refs.h
+++ b/refs.h
@@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem. The ref_transaction is freed by this function.
+ * If err is non-NULL we will add an error string to it to explain why
+ * the transaction failed.
Probably worth mentioning the error string doesn't end with a newline
so the caller knows how to use it.
With the whitespace fix and with or without the comment tweak,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/refs.c w/refs.c
index 64e8feb..2ca3169 100644
--- i/refs.c
+++ w/refs.c
@@ -3445,7 +3445,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-type, onerr);
if (!update-lock) {
if (err)
- strbuf_addf(err ,Cannot lock the ref '%s'.,
+ strbuf_addf(err, Cannot lock the ref '%s'.,
update-refname);
ret = 1;
goto cleanup;
diff --git i/refs.h w/refs.h
index ff87e14..d45212f 100644
--- i/refs.h
+++ w/refs.h
@@ -268,8 +268,8 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem. The ref_transaction is freed by this function.
- * If err is non-NULL we will add an error string to it to explain why
- * the transaction failed.
+ * If err is non-NULL we will append an error string (with no trailing
+ * newline) to it to explain why the transaction failed.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
const char *msg, struct strbuf *err,
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 04/42] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors
Ronnie Sahlberg wrote: Make ref_update_reject_duplicates return any error that occurs through a new strbuf argument. Sensible. The caller-visible effect would be that now ref_transaction_commit() can pass back a helpful error message through its err argument when asked to make multiple updates for the same ref. Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git grep -O -i: if the pager is 'less', pass the '-i' option
Hi, Junio C Hamano wrote: Stepan Kasal ka...@ucw.cz writes: --- a/builtin/grep.c +++ b/builtin/grep.c @@ -897,6 +897,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix) if (len 4 is_dir_sep(pager[len - 5])) pager += len - 4; +if (opt.ignore_case !strcmp(less, pager)) +string_list_append(path_list, -i); I have a feeling that this goes against the recent trend of not mucking with the expectation of the users on their pagers, if I recall correctly the arguments for dropping S from the default given to an unconfigured LESS environment variable. It's just missing an explanation. When command happens to be the magic string less, today git grep -Ocommand -epattern helpfully passes +/pattern to less so you can navigate through the results within a file using the n and shift+n keystrokes. Alas, that doesn't do the right thing for a case-insensitive match. For that case we should pass --IGNORE-CASE to less so that n and shift+n can move between results ignoring case in the pattern. (That's -I, not -i, because it ought to work even when the pattern contains capital letters.) git grep has other options that affect interpretation of the pattern which this patch does not help with: * -v / --ignore-match: probably should disable this feature of -O. * -E / --extended-regexp * -P / --perl-regexp * -F / --fixed-strings: ideally would auto-escape regex specials. * -epattern1 --or -epattern2 And git grep -Ovi has a similar bug, for which the fix is to add \c to the pattern instead of passing an -I option. But as is, it's an improvement, so (except that -i should be replaced by -I) it seems like a good change. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 05/42] update-ref.c: log transaction error from the update_ref
Hi, Ronnie Sahlberg wrote: --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) [...] @@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) [...] if (delete || no_deref || argc 0) usage_with_options(git_update_ref_usage, options); if (end_null) line_termination = '\0'; update_refs_stdin(); - ret = ref_transaction_commit(transaction, msg, NULL, - UPDATE_REFS_DIE_ON_ERR); - return ret; + if (ref_transaction_commit(transaction, msg, err, +UPDATE_REFS_QUIET_ON_ERR)) + die(%s, err.buf); Nice. I like this much more than passing a flag to each function to tell it how to handle errors. :) ref_transaction_commit didn't have any stray codepaths that return some other exit code instead of die()-ing with UPDATE_REFS_DIE_ON_ERR, so this should be safe as far as the exit code is concerned. The only danger would be that some codepath leaves 'err' alone and forgets to write a messages, so we die with fatal: Alas, it looks like this patch can do that. i. The call to update_ref_write can error out without updating the error string. ii. delete_ref_loose can print a message and then fail without updating the error string so the output looks like warning: unable to unlink .git/refs/heads/master.lock: Permission denied fatal: $ iii. repack_without_refs can similarly return an error error: Unable to create '/home/jrn/test/.git/packed-refs.lock: Permission denied error: cannot delete 'refs/heads/master' from packed refs fatal: $ iv. commit_lock_file in commit_packed_refs is silent on error. repack_without_refs probably intends to write a message in that case but doesn't :( I wish there were some way to automatically detect missed spots or make them stand out (like with the current return error() idiom a bare return -1 stands out). (i) is fixed by a later patch. It would be better to put that before this one for bisectability. I don't see fixes to (ii), (iii), and (iv) in the series yet from a quick glance. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 06/42] refs.c: make update_ref_write update a strbuf on failure
Ronnie Sahlberg wrote: Change update_ref_write to also update an error strbuf on failure. This makes the error available to ref_transaction_commit callers if the transaction failed due to update_ref_sha1/write_ref_sha1 failures. Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 07/42] refs.c: remove the onerr argument to ref_transaction_commit
Ronnie Sahlberg wrote: Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr argument any more. Remove the onerr argument from the ref_transaction_commit signature. Nice, and obviously correct. Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 08/42] refs.c: change ref_transaction_update() to do error checking and return status
Ronnie Sahlberg wrote: Update ref_transaction_update() do some basic error checking and return true on error. Update all callers to check ref_transaction_update() for error. There are currently no conditions in _update that will return error but there will be in the future. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/update-ref.c | 10 ++ refs.c | 9 +++-- refs.h | 10 +- 3 files changed, 18 insertions(+), 11 deletions(-) Revisiting comments from [1]: * When I call ref_transaction_update, what does it mean that I get a nonzero return value? Does it mean the _update failed and had no effect? What will I want to do next: should I try again or print an error and exit? Ideally I should be able to answer these questions by reading the signature of ref_transaction_update and the comment documenting it. The comment doesn't say anything about what errors mean here. * the error message change for the have_old !old_sha1 case (to add BUG: so users know the impossible has happened and translators know not to bother with it) seems to have snuck ahead into patch 28 (refs.c: add transaction.status and track OPEN/CLOSED/ERROR). * It would be easier to make sense of the error path (does the error message have enough information? Will the user be bewildered?) if there were an example of how ref_transaction_update can fail. There still doesn't seem to be one by the end of the series. The general idea still seems sensible. Thanks, Jonathan [1] http://thread.gmane.org/gmane.comp.version-control.git/246437/focus=247115 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 09/42] refs.c: change ref_transaction_create to do error checking and return status
Ronnie Sahlberg wrote: Do basic error checking in ref_transaction_create() and make it return status. Update all callers to check the result of ref_transaction_create() There are currently no conditions in _create that will return error but there will be in the future. Same concerns as with _update: [...] +++ b/builtin/update-ref.c @@ -225,7 +225,9 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next) if (*next != line_termination) die(create %s: extra input: %s, refname, next); - ref_transaction_create(transaction, refname, new_sha1, update_flags); + if (ref_transaction_create(transaction, refname, new_sha1, +update_flags)) + die(failed transaction create for %s, refname); If it were ever triggered, the message error: some bad thing fatal: failed transaction create for refs/heads/master looks overly verbose and unclear. Something like fatal: cannot create ref refs/heads/master: some bad thing might work better. It's hard to tell without an example in mind. [...] @@ -3353,18 +3353,23 @@ void ref_transaction_create(struct ref_transaction *transaction, [...] - assert(!is_null_sha1(new_sha1)); + if (!new_sha1 || is_null_sha1(new_sha1)) + die(create ref with null new_sha1); One less 'assert' is nice. :) As with _update, the message should start with BUG: to make it clear to users and translators that this should never happen, even with malformed user input. That gets corrected in patch 28 but it's clearer to include it from the start. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 10/42] refs.c: ref_transaction_delete to check for error and return status
Ronnie Sahlberg wrote: Change ref_transaction_delete() to do basic error checking and return status. Update all callers to check the return for ref_transaction_delete() There are currently no conditions in _delete that will return error but there will be in the future. Signed-off-by: Ronnie Sahlberg sahlb...@google.com Same comments as _delete and _update. :) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 11/42] tag.c: use ref transactions when doing updates
Ronnie Sahlberg wrote: --- a/builtin/tag.c +++ b/builtin/tag.c @@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix) if (annotate) create_tag(object, tag, buf, opt, prev, object); - lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL); - if (!lock) - die(_(%s: cannot lock the ref), ref.buf); - if (write_ref_sha1(lock, object, NULL) 0) - die(_(%s: cannot update the ref), ref.buf); + transaction = ref_transaction_begin(); + if (!transaction || + ref_transaction_update(transaction, ref.buf, object, prev, +0, !is_null_sha1(prev)) || + ref_transaction_commit(transaction, NULL, err)) + die(_(%s: cannot update the ref: %s), ref.buf, err.buf); Makes sense for the _update and _commit case. (BTW, why is have_old a separate boolean instead of a bit in flags?) For the _begin() case, can ref_transaction_begin() ever fail? xcalloc die()s on allocation failure. So I think it's fine to assume transaction is non-null (i.e., drop the !transaction condition), or if you want to be defensive, then label it as a bug --- e.g.: if (!transaction) die(BUG: ref_transaction_begin() returned NULL?); Otherwise if ref_transaction_begin regresses in the future and this case is tripped then the message would be fatal: refs/tags/v1.0: cannot update the ref: which is not as obvious an indicator that the user should contact the mailing list. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 12/42] replace.c: use the ref transaction functions for updates
Ronnie Sahlberg wrote: [...] +++ b/builtin/replace.c [...] @@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const char *replace_ref, else if (!force) die(replace ref '%s' already exists, ref); - lock = lock_any_ref_for_update(ref, prev, 0, NULL); - if (!lock) - die(%s: cannot lock the ref, ref); - if (write_ref_sha1(lock, repl, NULL) 0) - die(%s: cannot update the ref, ref); + transaction = ref_transaction_begin(); + if (!transaction || + ref_transaction_update(transaction, ref, repl, prev, +0, !is_null_sha1(prev)) || + ref_transaction_commit(transaction, NULL, err)) + die(_(%s: failed to replace ref: %s), ref, err.buf); Same question about the !transaction case. This makes the message translated, which is a nice change but not mentioned in the commit message. (Generally speaking, I don't mind either way about adding or not adding _() to new messages in files that have not already undergone a pass of marking everything for translation.) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 13/42] commit.c: use ref transactions for updates
Ronnie Sahlberg wrote:
[...]
+++ b/builtin/commit.c
@@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const
char *prefix)
[...]
@@ -1667,16 +1668,6 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_release(author_ident);
free_commit_extra_headers(extra);
- ref_lock = lock_any_ref_for_update(HEAD,
-!current_head
-? NULL
-: current_head-object.sha1,
-0, NULL);
- if (!ref_lock) {
- rollback_index_files();
- die(_(cannot lock HEAD ref));
- }
-
nl = strchr(sb.buf, '\n');
if (nl)
strbuf_setlen(sb, nl + 1 - sb.buf);
else
strbuf_addch(sb, '\n');
strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(sb, strlen(reflog_msg), : , 2);
- if (write_ref_sha1(ref_lock, sha1, sb.buf) 0) {
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, HEAD, sha1,
+current_head ?
+current_head-object.sha1 : NULL,
+0, !!current_head) ||
+ ref_transaction_commit(transaction, sb.buf, err)) {
rollback_index_files();
- die(_(cannot update HEAD ref));
+ die(_(HEAD: cannot update ref: %s), err.buf);
Same question about !transaction (it also applies to later patches but I
won't mention it any more).
The error message changed from
fatal: cannot lock HEAD ref
to
fatal: HEAD: cannot update ref: Cannot lock the ref 'HEAD'.
Does the message from ref_transaction_commit always say what ref
was being updated when it failed? If so, it's tempting to just use
the message as-is:
fatal: Cannot lock the ref 'HEAD'
If the caller should add to the message, it could say something about
the context --- e.g.,
fatal: cannot update HEAD with new commit: cannot lock the ref 'HEAD'
Looking at that,
die(%s, err.buf)
seems simplest since even if git commit was being called in a loop,
it's already clear that git was trying to lock HEAD to advance it.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/10] git-submodule.sh: don't use the -a or -b option with the test command
Elia Pinto wrote:
Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted as follow:
[... many lines snipped ...]
This is a very long description, and it doesn't leave me excited by
the change.
Is there some potential bug this prevents, or is it just a style
fix? If the latter, do we have a way of checking for new examples
of the same thing to avoid having to repeat the same patch again in
the future?
Are there any platforms that were broken which this fixes? Even
posh seems to understand -a and -o.
Nowadays Documentation/CodingGuidelines says
- Fixing style violations while working on a real change as a
preparatory clean-up step is good, but otherwise avoid useless code
churn for the sake of conforming to the style.
Once it _is_ in the tree, it's not really worth the patch noise to
go and fix it up.
Cf. http://article.gmane.org/gmane.linux.kernel/943020
which I think goes too far (some patterns really are error prone
or distracting and it can be worth fixing them tree-wide), but it
makes a reasonable case that an idiom not being preferred in the
style guide is not *on its own* enough reason to change it.
Perhaps something like the following would work?
tree-wide: convert test -a/-o to and ||
The interaction with unary operators and operator precedence
for and || are better known than -a and -o, and for that
reason we prefer them. Replace all existing instances in git
of -a and -o to save readers from the burden of thinking
about such things.
Add a check-non-portable-shell.pl to avoid more instances of
test -a and -o arising in the future.
[...]
- test $status = D -o $status = T echo $sm_path
continue
+ ( test $status = D || test $status = T ) echo
$sm_path continue
There's no need for a subshell for this. Better to use a block:
{
test $status = D ||
test $status = T
}
echo $sm_path
continue
or an if statement:
if test $status = D || test $status = T
then
echo $sm_path
continue
fi
or case:
case $status in
D|T)
echo $sm_path
continue
;;
esac
Hope that helps,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 11/42] tag.c: use ref transactions when doing updates
Ronnie Sahlberg wrote: Instead of the suggestions above, would you accept an alternative approach where I would add an err argument to ref_transaction_begin() instead? For a hypothetical mysql backend, this could then do something like : [...] fatal: refs/heads/master: cannot update the ref: failed to connect to mysql database ... Yes, sounds like a good thing to do. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 09/42] refs.c: change ref_transaction_create to do error checking and return status
Ronnie Sahlberg wrote: On Wed, May 14, 2014 at 5:04 PM, Jonathan Nieder jrnie...@gmail.com wrote: If it were ever triggered, the message error: some bad thing fatal: failed transaction create for refs/heads/master looks overly verbose and unclear. Something like fatal: cannot create ref refs/heads/master: some bad thing I changed it to : die(cannot create ref '%s', refname); But it would still mean you would have error: some bad thing fatal: cannot create 'refs/heads/master' To make it better we have to wait until the end of the second patch series, ref-transactions-next where we will have an err argument to _update/_create/_delete too and thus we can do this from update-ref.c : if (transaction_create_sha1(transaction, refname, new_sha1, update_flags, msg, err)) die(%s, err.buf); Thanks. Sounds good. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 14/42] sequencer.c: use ref transactions for all ref updates
Ronnie Sahlberg wrote:
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
static int fast_forward_to(const unsigned char *to, const unsigned char
*from,
int unborn, struct replay_opts *opts)
{
- struct ref_lock *ref_lock;
+ struct ref_transaction *transaction;
struct strbuf sb = STRBUF_INIT;
- int ret;
+ struct strbuf err = STRBUF_INIT;
read_cache();
if (checkout_fast_forward(from, to, 1))
exit(1); /* the callee should have complained already */
- ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from,
-0, NULL);
- if (!ref_lock)
- return error(_(Failed to lock HEAD during fast_forward_to));
strbuf_addf(sb, %s: fast-forward, action_name(opts));
- ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+ transaction = ref_transaction_begin();
+ if ((!transaction ||
+ ref_transaction_update(transaction, HEAD, to, from,
+0, !unborn)) ||
These parentheses make it harder to parse. Other patches in this
series do
if (!transaction ||
ref_transaction_update(...) ||
ref_transaction_commit(...)) {
so this could do
if (!transaction ||
ref_transaction_update(...) ||
(ref_transaction_commit(...) !(transaction = NULL))) {
+ (ref_transaction_commit(transaction, sb.buf, err)
+ !(transaction = NULL))) {
+ ref_transaction_rollback(transaction);
Earlier patches in the series didn't bother rolling back. Should they
have?
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git grep -O -i: if the pager is 'less', pass the '-i' option
Hi, Johannes Schindelin wrote: On Wed, 14 May 2014, Junio C Hamano wrote: Spot on. The change, especially with -I, makes sense. Except that it was not tested with -I. If you change it that way and it stops working on Windows, it's useless to me. Are you saying that less on Windows doesn't have an -I option? version.c tells me it was added in v266 (1994-12-26). If -I' is in fact broken on Windows, that would be useful to know, but it's not clear to me. Or if I have misunderstood what git grep -i -Oless -eGit_ is supposed to do, that would help, too. Puzzled, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git grep -O -i: if the pager is 'less', pass the '-i' option
Johannes Schindelin wrote: On Wed, 14 May 2014, Jeff King wrote: We've already found the lines of interest to the user. It would be nice if we could somehow point the pager at them by number, rather than repeating the (slightly incompatible) search. FWIW it is exactly that type of I want your patch to do more than you do type of comments that makes it impossible for myself to contribute patches anymore. I think you're overreacting to Peff's It would be nice. It is a way of talking about where this lies in a design space that also includes the git-jump tool that Peff worked on. Maybe the tools can learn from each other. It is not a reason not to apply the patch. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 00/44] Use ref transactions for all ref updates
Ronnie Sahlberg wrote:
This patch series is based on next and expands on the transaction API.
Thanks. Will pick up in v8 where I left off with v6.
Applies with just one minor conflict on top of a merge of
mh/ref-transaction, rs/ref-update-check-errors-early, and
rs/reflog-exists. Here's an interdiff against version 6 for those
following along.
Version 8:
- Updates after review by JN
- Improve commit messages
- Add a patch that adds an err argument to repack_without_refs
- Add a patch that adds an err argument to delete_loose_ref
- Better document that a _update/_delete/_create failure means the whole
transaction has failed and needs rollback.
Version 7:
- Updated commit messages per JNs review comments.
- Changed REF_ISPRUNING and REF_ISPACKONLY to be private flags and not
exposed through refs.h
diff --git a/builtin/commit.c b/builtin/commit.c
index 0f4e1fc..07ccc97 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1684,7 +1684,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
0, !!current_head, sb.buf) ||
ref_transaction_commit(transaction, err)) {
rollback_index_files();
- die(_(HEAD: cannot update ref: %s), err.buf);
+ die(%s, err.buf);
}
ref_transaction_free(transaction);
diff --git a/builtin/replace.c b/builtin/replace.c
index 47c360c..952b589 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -163,7 +163,7 @@ static int replace_object(const char *object_ref, const
char *replace_ref,
ref_transaction_update(transaction, ref, repl, prev,
0, !is_null_sha1(prev), NULL) ||
ref_transaction_commit(transaction, err))
- die(_(%s: failed to replace ref: %s), ref, err.buf);
+ die(%s: failed to replace ref: %s, ref, err.buf);
ref_transaction_free(transaction);
return 0;
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index c5aff92..bd7e96f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -228,7 +228,7 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
if (ref_transaction_create(transaction, refname, new_sha1,
update_flags, msg))
- die(failed transaction create for %s, refname);
+ die(cannot create ref '%s', refname);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 302a2b3..ed93b75 100644
--- a/refs.c
+++ b/refs.c
@@ -29,6 +29,15 @@ static inline int bad_ref_char(int ch)
return 0;
}
+/** Used as a flag to ref_transaction_delete when a loose ref is beeing
+ * pruned.
+ */
+#define REF_ISPRUNING 0x0100
+/** Deletion of a ref that only exists as a packed ref in which case we do not
+ * need to lock the loose ref during the transaction.
+ */
+#define REF_ISPACKONLY 0x0200
+
/*
* Try to read one refname component from the front of refname. Return
* the length of the component found, or -1 if the component is not
@@ -2447,12 +2456,12 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-static int repack_without_refs(const char **refnames, int n)
+static int repack_without_refs(const char **refnames, int n, struct strbuf
*err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int i, removed = 0;
+ int i, ret, removed = 0;
/* Look for a packed ref */
for (i = 0; i n; i++)
@@ -2465,6 +2474,9 @@ static int repack_without_refs(const char **refnames, int
n)
if (lock_packed_refs(0)) {
unable_to_lock_error(git_path(packed-refs), errno);
+ if (err)
+ strbuf_addf(err, cannot delete '%s' from packed refs,
+ refnames[i]);
return error(cannot delete '%s' from packed refs,
refnames[i]);
}
packed = get_packed_refs(ref_cache);
@@ -2490,20 +2502,28 @@ static int repack_without_refs(const char **refnames,
int n)
}
/* Write what remains */
- return commit_packed_refs();
+ ret = commit_packed_refs();
+ if (ret err)
+ strbuf_addf(err, unable to overwrite old ref-pack file);
+ return ret;
}
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
/* loose */
- int err, i = strlen(lock-lk-filename) - 5; /* .lock */
+ int res, i = strlen(lock-lk-filename) - 5; /* .lock */
lock-lk-filename[i] = 0;
- err = unlink_or_warn(lock-lk-filename);
+ res = unlink_or_warn(lock-lk-filename);
Re: [PATCH v8 01/44] refs.c: constify the sha arguments for ref_transaction_create|delete|update
Ronnie Sahlberg wrote: ref_transaction_create|delete|update has no need to modify the sha1 arguments passed to it so it should use const unsigned char* instead of unsigned char*. Obviously good. Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 02/44] refs.c: allow passing NULL to ref_transaction_free
Ronnie Sahlberg wrote:
Allow ref_transaction_free to be called with NULL and as a result allow
ref_transaction_rollback to be called for a NULL transaction.
This allows us to write code that will
if ( (!transaction ||
ref_transaction_update(...)) ||
(ref_transaction_commit(...) !(transaction = NULL)) {
ref_transaction_rollback(transaction);
...
}
In this case transaction is reset to NULL IFF ref_transaction_commit() was
invoked and thus the rollback becomes ref_transaction_rollback(NULL) which
is safe. IF the conditional triggered prior to ref_transaction_commit()
then transaction is untouched and then ref_transaction_rollback(transaction)
will rollback the failed transaction.
I still think these last two paragraphs confuse more than enlighten
here. There's plenty of time to explain them in the patch that uses
that code.
I'd just say something like
Allow ref_transaction_free(NULL) and hence
ref_transaction_rollback(NULL)
as no-ops.
This makes ref_transaction_rollback easier to use and more similar to
plain 'free'.
And maybe:
In particular, it lets us rollback unconditionally as part of cleanup
code after setting 'transaction = NULL' if a transaction has been
committed or rolled back already.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 04/44] refs.c: add an err argument to repack_without_refs
Ronnie Sahlberg wrote:
--- a/refs.c
+++ b/refs.c
@@ -2427,12 +2427,12 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-static int repack_without_refs(const char **refnames, int n)
+static int repack_without_refs(const char **refnames, int n, struct strbuf
*err)
Should this also get an onerr flag to suppress the message to stderr,
or unconditionally suppress the message to stderr when err != NULL?
[...]
@@ -2445,6 +2445,9 @@ static int repack_without_refs(const char **refnames,
int n)
if (lock_packed_refs(0)) {
unable_to_lock_error(git_path(packed-refs), errno);
+ if (err)
+ strbuf_addf(err, cannot delete '%s' from packed refs,
+ refnames[i]);
unable_to_lock_error is able to come up with a message with more
detail (path so the sysadmin can hunt down the problem even if this
was run e.g. from a cronjob where the path is not obvious, errno
hinting at the nature of the problem).
[...]
@@ -2470,12 +2473,15 @@ static int repack_without_refs(const char **refnames,
int n)
}
/* Write what remains */
- return commit_packed_refs();
+ ret = commit_packed_refs();
+ if (ret err)
+ strbuf_addf(err, unable to overwrite old ref-pack file);
After commit_lock_file sets errno, amazingly no one clobbers it
until we get to this point. The only calls in between are to
free().
It would be nice to make that more explicit in commit_packed_refs:
int save_errno;
...
if (commit_lock_file(packed_ref_cache-lock)) {
save_errno = errno;
error = -1;
}
packed_ref_cache-lock = NULL;
release_packed_ref_cache(packed_ref_cache);
errno = save_errno;
return error;
Even without that, this message could include strerror(errno).
+ return ret;
}
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 06/44] refs.c: add an err argument ro delete_loose_ref
Ronnie Sahlberg wrote:
[Subject: refs.c: add an err argument ro delete_loose_ref]
s/ro/to/
s/delete_loose_ref/delete_ref_loose/
--- a/refs.c
+++ b/refs.c
@@ -2484,17 +2484,22 @@ static int repack_without_ref(const char *refname)
return repack_without_refs(refname, 1, NULL);
}
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
Should this get an onerr flag to suppress the message to stderr
or unconditionally suppress it when err != NULL?
[...]
lock-lk-filename[i] = 0;
- err = unlink_or_warn(lock-lk-filename);
+ res = unlink_or_warn(lock-lk-filename);
It seems like in the new error handling scheme there should be a new
variant on wrapper.c's warn_if_unremovable:
static int add_err_if_unremovable(const char *op, const char *file,
struct strbuf *err, int rc)
{
int err = errno;
if (rc 0 err != ENOENT) {
strbuf_addf(err, unable to %s %s: %s,
op, file, strerror(errno));
errno = err;
}
return rc;
}
static int unlink_or_err(const char *file, struct strbuf *err)
{
return add_err_if_unremovable(unlink, file, err,
unlink(file));
}
res = unlink_or_err(lock-lk-filename, err);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 08/44] update-ref.c: log transaction error from the update_ref
Ronnie Sahlberg wrote: Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is returned to print a log message if/after the transaction fails. Signed-off-by: Ronnie Sahlberg sahlb...@google.com All error paths in _commit add to the error string now, so the only effect should be to add the missing error message when commiting packed-refs fails (thanks for fixing that) and to change some one-line errors to two-line. Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 10/44] refs.c: change ref_transaction_update() to do error checking and return status
Ronnie Sahlberg wrote: Update ref_transaction_update() do some basic error checking and return non-zero on error. Update all callers to check ref_transaction_update() for error. There are currently no conditions in _update that will return error but there will be in the future. Probably worth passing a 'struct strbuf *err' argument. Then callers can do die(%s, err.buf); and the error message can say which ref and whether we were trying to create a ref, or delete one, or whatever. --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -197,8 +197,9 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next) if (*next != line_termination) die(update %s: extra input: %s, refname, next); - ref_transaction_update(transaction, refname, new_sha1, old_sha1, -update_flags, have_old); + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1, +update_flags, have_old)) + die(update %s: failed, refname); This could say die(update %s: %s, refname, err.buf); to give context about which command it was trying to execute. [...] @@ -286,8 +287,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next) if (*next != line_termination) die(verify %s: extra input: %s, refname, next); - ref_transaction_update(transaction, refname, new_sha1, old_sha1, -update_flags, have_old); + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1, +update_flags, have_old)) + die(failed transaction update for %s, refname); And this could say die(verify %s: %s, refname, err.buf); [...] --- a/refs.h +++ b/refs.h @@ -242,12 +242,15 @@ void ref_transaction_rollback(struct ref_transaction *transaction); * be deleted. If have_old is true, then old_sha1 holds the value * that the reference should have had before the update, or zeros if * it must not have existed beforehand. + * Function returns 0 on success and non-zero on failure. A failure to update + * means that the transaction as a whole has failed and will need to be + * rolled back. + */ Thanks for this documentation. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 11/44] refs.c: change ref_transaction_create to do error checking and return status
Ronnie Sahlberg wrote: Do basic error checking in ref_transaction_create() and make it return non-zero on error. Same thoughts as _update(). Basic idea is good but would be nice to have a 'struct strbuf *err' parameter. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 12/44] refs.c: ref_transaction_delete to check for error and return status
Ronnie Sahlberg wrote: Change ref_transaction_delete() to do basic error checking and return non-zero of error. Likewise: a 'struct strbuf *err' would make nicer error messages possible. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 13/44] tag.c: use ref transactions when doing updates
Ronnie Sahlberg wrote: --- a/builtin/tag.c +++ b/builtin/tag.c @@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix) if (annotate) create_tag(object, tag, buf, opt, prev, object); - lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL); - if (!lock) - die(_(%s: cannot lock the ref), ref.buf); - if (write_ref_sha1(lock, object, NULL) 0) - die(_(%s: cannot update the ref), ref.buf); + transaction = ref_transaction_begin(); + if (!transaction || + ref_transaction_update(transaction, ref.buf, object, prev, +0, !is_null_sha1(prev)) || + ref_transaction_commit(transaction, NULL, err)) + die(_(%s: cannot update the ref: %s), ref.buf, err.buf); The error string says what ref it was trying to update, so die(%s, err.buf); should be enough. (E.g., fatal: refs/tags/v1.0: cannot lock the ref would become fatal: Cannot lock the ref 'refs/tags/v1.0'. instead of fatal: refs/tags/v1.0: cannot update the ref: Cannot lock the ref 'refs/tags/v1.0'. .) Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 14/44] replace.c: use the ref transaction functions for updates
Ronnie Sahlberg wrote: --- a/builtin/replace.c +++ b/builtin/replace.c [...] @@ -156,11 +157,12 @@ static int replace_object_sha1(const char *object_ref, else if (!force) die(replace ref '%s' already exists, ref); - lock = lock_any_ref_for_update(ref, prev, 0, NULL); - if (!lock) - die(%s: cannot lock the ref, ref); - if (write_ref_sha1(lock, repl, NULL) 0) - die(%s: cannot update the ref, ref); + transaction = ref_transaction_begin(); + if (!transaction || + ref_transaction_update(transaction, ref, repl, prev, +0, !is_null_sha1(prev)) || + ref_transaction_commit(transaction, NULL, err)) + die(%s: failed to replace ref: %s, ref, err.buf); This would write fatal: refs/replace/09c779943364d893c190066c385e6112af421db3: failed to replace ref: Cannot lock the ref 'refs/replace/09c779943364d893c190066c385e6112af421db3'. Perhaps something like $ git replace foo bar fatal: replace foo: Cannot lock the ref 'refs/replace/09c779943364d893c190066c385e6112af421db3'. would make sense (die(replace %s: %s, object_ref, err.buf)). Plain $ git replace foo bar fatal: Cannot lock the ref 'refs/replace/09c779943364d893c190066c385e6112af421db3'. also seems fine (die(%s, err.buf)). Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
