* Re: [PATCH 09/14] imap-send.c: remove namespace fields from struct imap
From: Michael Haggerty @ 2013-01-14 9:31 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Jeff King, git
In-Reply-To: <20130114064347.GH3125@elie.Belkin>
On 01/14/2013 07:43 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
>
> [...]
>> @@ -722,9 +667,9 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
>> }
>>
>> if (!strcmp("NAMESPACE", arg)) {
>> - imap->ns_personal = parse_list(&cmd);
>> - imap->ns_other = parse_list(&cmd);
>> - imap->ns_shared = parse_list(&cmd);
>> + skip_list(&cmd);
>> + skip_list(&cmd);
>> + skip_list(&cmd);
>
> This codepath would only be triggered if we emit a NAMESPACE command,
> right? If I am understanding correctly, a comment could make this
> less mystifying:
>
> /* rfc2342 NAMESPACE response. */
> skip_list(&cmd); /* Personal mailboxes */
> skip_list(&cmd); /* Others' mailboxes */
> skip_list(&cmd); /* Shared mailboxes */
Yes, the comments are useful.
> Though that's probably academic since hopefully this if branch
> will die soon. :)
Not by my hand :-) Maybe somebody more familiar with the IMAP protocol
wants to take the code culling further...
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 00/14] Remove unused code from imap-send.c
From: Michael Haggerty @ 2013-01-14 9:33 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Jeff King, git
In-Reply-To: <20130114065757.GK3125@elie.Belkin>
On 01/14/2013 07:57 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
>
>> imap-send.c | 286 +++++++++---------------------------------------------------
>> 1 file changed, 39 insertions(+), 247 deletions(-)
>
> See my replies for comments on patches 1, 6, 9, 11, and 12. The rest
> are
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> The series is tasteful and easy to follow and it's hard to argue with
> the resulting code reduction. Thanks for a pleasant read.
Thanks for your careful review. I will re-roll the patch series as soon
as I get the chance.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 2/8] git_remote_helpers: fix input when running under Python 3
From: John Keeping @ 2013-01-14 9:47 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git, Eric S. Raymond, Felipe Contreras, Sverre Rabbelier
In-Reply-To: <50F38E12.6090207@alum.mit.edu>
On Mon, Jan 14, 2013 at 05:48:18AM +0100, Michael Haggerty wrote:
> On 01/13/2013 05:17 PM, John Keeping wrote:
>> On Sun, Jan 13, 2013 at 04:26:39AM +0100, Michael Haggerty wrote:
>>> On 01/12/2013 08:23 PM, John Keeping wrote:
>>>> Although 2to3 will fix most issues in Python 2 code to make it run under
>>>> Python 3, it does not handle the new strict separation between byte
>>>> strings and unicode strings. There is one instance in
>>>> git_remote_helpers where we are caught by this.
>>>>
>>>> Fix it by explicitly decoding the incoming byte string into a unicode
>>>> string. In this instance, use the locale under which the application is
>>>> running.
>>>>
>>>> Signed-off-by: John Keeping <john@keeping.me.uk>
>>>> ---
>>>> git_remote_helpers/git/importer.py | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py
>>>> index e28cc8f..6814003 100644
>>>> --- a/git_remote_helpers/git/importer.py
>>>> +++ b/git_remote_helpers/git/importer.py
>>>> @@ -20,7 +20,7 @@ class GitImporter(object):
>>>> """Returns a dictionary with refs.
>>>> """
>>>> args = ["git", "--git-dir=" + gitdir, "for-each-ref", "refs/heads"]
>>>> - lines = check_output(args).strip().split('\n')
>>>> + lines = check_output(args).decode().strip().split('\n')
>>>> refs = {}
>>>> for line in lines:
>>>> value, name = line.split(' ')
>>>>
>>>
>>> Won't this change cause an exception if the branch names are not all
>>> valid strings in the current locale's encoding? I don't see how this
>>> assumption is justified (e.g., see git-check-ref-format(1) for the rules
>>> governing reference names).
>>
>> Yes it will. The problem is that for Python 3 we need to decode the
>> byte string into a unicode string, which means we need to know what
>> encoding it is.
>>
>> I don't think we can just say "git-for-each-ref will print refs in
>> UTF-8" since AFAIK git doesn't care what encoding the refs are in - I
>> suspect that's determined by the filesystem which in the end probably
>> maps to whatever bytes the shell fed git when the ref was created.
>>
>> That's why I chose the current locale in this case. I'm hoping someone
>> here will correct me if we can do better, but I don't see any way of
>> avoiding choosing some encoding here if we want to support Python 3
>> (which I think we will, even if we don't right now).
>
> I'm not just trying to be a nuisance here;
You're not being - I think this is a difficult issue and I don't know
myself what the right answer is.
> I'm struggling myself to
> understand how a program that cares about strings-vs-bytes (e.g., a
> Python3 script) should coexist with a program that doesn't (e.g., git
> [1]). I think this will become a big issue if my Python version of the
> commit email script ever gets integrated and then made compatible with
> Python3.
>
> You claim "for Python 3 we need to decode the byte string into a unicode
> string". I understand that Python 3 strings are Unicode, but why/when
> is it necessary to decode data into a Unicode string as opposed to
> leaving it as a byte sequence?
>
> In this particular case (from a cursory look over the code) it seems to
> me that (1) decoding to Unicode will sometimes fail for data that git
> considers valid and (2) there is no obvious reason that the data cannot
> be processed as byte sequences.
I've been thinking about this overnight and I think you're right that
treating them as byte strings in Python is most correct. Having said
that, when I'm programming in Python I would find it quite surprising
that I had to treat ref strings specially - and as soon as I want to use
one in a string context (e.g. printing it as part of a message to the
user) I'm back to the same problem.
So I think we should try to solve the problem once rather than forcing
everyone who wants to use the library to solve it individually. I just
wish it was obvious what we should do!
John
^ permalink raw reply
* Re: [BUG] Bug in git stash
From: Johannes Sixt @ 2013-01-14 11:45 UTC (permalink / raw)
To: Nikolay Frantsev; +Cc: git
In-Reply-To: <CAOJsraHfKRoK1G8tA2ROUH6qt-q0kG6WesZr5enOeeqFF_AC=w@mail.gmail.com>
Am 1/14/2013 10:18, schrieb Nikolay Frantsev:
> nikolay@localhost:~/Desktop/git-stash_bug/bug$ git status
> # On branch master
> # Changes to be committed:
> # (use "git reset HEAD <file>..." to unstage)
> #
> # new file: 3
> #
> # Changes not staged for commit:
> # (use "git add <file>..." to update what will be committed)
> # (use "git checkout -- <file>..." to discard changes in working directory)
> #
> # modified: 1
> # modified: 2
> #
> nikolay@localhost:~/Desktop/git-stash_bug/bug$ git stash save --keep-index one
> Saved working directory and index state On master: one
> HEAD is now at 7e495f9 files added
...
> nikolay@localhost:~/Desktop/git-stash_bug/bug$ git stash pop stash@{1}
> # On branch master
> # Changes to be committed:
> # (use "git reset HEAD <file>..." to unstage)
> #
> # new file: 3
> #
> # Changes not staged for commit:
> # (use "git add <file>..." to update what will be committed)
> # (use "git checkout -- <file>..." to discard changes in working directory)
> #
> # modified: 1
> # modified: 2
> #
> Dropped stash@{1} (7926ab7285753c179a368a3a7e8ebfb0f39d0437)
>
> Why there a new empty file named 3?
It is by design. --keep-index only achieves that your staged changes are
not reverted, but nevertheless all changes are stashed away. Therefore,
when you later apply the stash, you also get back the modified index.
-- Hannes
^ permalink raw reply
* Error:non-monotonic index after failed recursive "sed" command
From: George Karpenkov @ 2013-01-14 11:40 UTC (permalink / raw)
To: git
Hi All,
I've managed to corrupt my very valuable repository with a recursive
sed which went wrong.
I wanted to convert all tabs to spaces with the following command:
find ./ -name '*.*' -exec sed -i 's/\t/ /g' {} \;
I think that has changed not only the files in the repo, but the data
files in .git directory itself. As a result, my index became
corrupted, and almost every single command dies:
> git log
error: non-monotonic index
.git/objects/pack/pack-314b1944adebea645526b6724b2044c1313241f5.idx
error: non-monotonic index
.git/objects/pack/pack-75c95b0defe1968b61e4f4e1ab7040d35110bfdc.idx
...
error: non-monotonic index
.git/objects/pack/pack-3da0da48d05140b55f4af1cf87c55a2d7898bdd5.idx
fatal: bad object HEAD
Output for git fsck is even worse:
> git fsck
error: non-monotonic index
.git/objects/pack/pack-434f8445672a92f123accffce651bdb693bd8fcb.idx
...
error: non-monotonic index
.git/objects/pack/pack-0c9d5ae4e2b46dd78dace7533adf6cdfe10326ef.idx
error: non-monotonic index
.git/objects/pack/pack-e8bd5c7f85e96e7e548a62954a8f7c7f223ba9e0.idx
Segmentation fault (core dumped)
Any advice? I've lost about 2 weeks worth of work.
Is there anything better I can try to do other then trying to
reconstruct the data manually from git blobs?
^ permalink raw reply
* Re: Error:non-monotonic index after failed recursive "sed" command
From: Matthieu Moy @ 2013-01-14 12:06 UTC (permalink / raw)
To: George Karpenkov; +Cc: git
In-Reply-To: <CAMoGvRKkSZqcoGtiebu6tuPndzOjQ1=JgQHb+iusAHpUbA2HbA@mail.gmail.com>
George Karpenkov <george@metaworld.ru> writes:
> Hi All,
>
> I've managed to corrupt my very valuable repository with a recursive
> sed which went wrong.
> I wanted to convert all tabs to spaces with the following command:
>
> find ./ -name '*.*' -exec sed -i 's/\t/ /g' {} \;
Clearly, this is a dangerous command as it impacts .git/. However, Git
partially protects you from this kind of error, since object files and
pack files are read-only by default.
My obvious first advice is: make backups of your corrupted repository.
Yes, I said backup_s_, better safe than sorry.
Then, the errors you get are in *.idx files, which are basically index
for pack files, for quicker access. You can try removing these files,
and then running "git index-pack" on each pack file, like
$ rm .git/objects/pack/pack-*.idx
$ git index-pack .git/objects/pack/pack-4745076928ca4df932a3727b8cc25e83574560bb.pack
4745076928ca4df932a3727b8cc25e83574560bb
$ git index-pack .git/objects/pack/pack-c74a6514f653d0269cdcdf9c1c102d326706bbda.pack
c74a6514f653d0269cdcdf9c1c102d326706bbda
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: Error:non-monotonic index after failed recursive "sed" command
From: John Keeping @ 2013-01-14 12:12 UTC (permalink / raw)
To: Matthieu Moy; +Cc: George Karpenkov, git
In-Reply-To: <vpq38y42clj.fsf@grenoble-inp.fr>
On Mon, Jan 14, 2013 at 01:06:16PM +0100, Matthieu Moy wrote:
> George Karpenkov <george@metaworld.ru> writes:
>> I've managed to corrupt my very valuable repository with a recursive
>> sed which went wrong.
>> I wanted to convert all tabs to spaces with the following command:
>>
>> find ./ -name '*.*' -exec sed -i 's/\t/ /g' {} \;
>
> Clearly, this is a dangerous command as it impacts .git/. However, Git
> partially protects you from this kind of error, since object files and
> pack files are read-only by default.
>
> My obvious first advice is: make backups of your corrupted repository.
> Yes, I said backup_s_, better safe than sorry.
Having backed up the corrupt state, another option is to just try
running the reverse command:
find .git -name '*.*' -exec sed -i 's/ /\t/g' {} \;
I had a quick grep over some pack indices here and ' ' doesn't occur
in any of mine whereas '\t' is very common so you may find that the only
' ' sequences are the ones you introduced.
John
^ permalink raw reply
* Re: Error:non-monotonic index after failed recursive "sed" command
From: Johannes Sixt @ 2013-01-14 12:21 UTC (permalink / raw)
To: George Karpenkov; +Cc: git
In-Reply-To: <CAMoGvRKkSZqcoGtiebu6tuPndzOjQ1=JgQHb+iusAHpUbA2HbA@mail.gmail.com>
Am 1/14/2013 12:40, schrieb George Karpenkov:
> I've managed to corrupt my very valuable repository with a recursive
> sed which went wrong.
> I wanted to convert all tabs to spaces with the following command:
>
> find ./ -name '*.*' -exec sed -i 's/\t/ /g' {} \;
>
> I think that has changed not only the files in the repo, but the data
> files in .git directory itself. As a result, my index became
> corrupted, and almost every single command dies:
>
>> git log
> error: non-monotonic index
> ..git/objects/pack/pack-314b1944adebea645526b6724b2044c1313241f5.idx
> error: non-monotonic index
> ..git/objects/pack/pack-75c95b0defe1968b61e4f4e1ab7040d35110bfdc.idx
> ....
> error: non-monotonic index
> ..git/objects/pack/pack-3da0da48d05140b55f4af1cf87c55a2d7898bdd5.idx
> fatal: bad object HEAD
>
> Output for git fsck is even worse:
>
>> git fsck
> error: non-monotonic index
> ..git/objects/pack/pack-434f8445672a92f123accffce651bdb693bd8fcb.idx
> ....
> error: non-monotonic index
> ..git/objects/pack/pack-0c9d5ae4e2b46dd78dace7533adf6cdfe10326ef.idx
> error: non-monotonic index
> ..git/objects/pack/pack-e8bd5c7f85e96e7e548a62954a8f7c7f223ba9e0.idx
> Segmentation fault (core dumped)
>
> Any advice? I've lost about 2 weeks worth of work.
> Is there anything better I can try to do other then trying to
> reconstruct the data manually from git blobs?
First things first: MAKE A COPY OF THE CURRENT STATE of the repository,
including the worktree, so that you have something to go back to if things
get worse later. (At the very least, we want to debug the segfault that we
see above.)
So far that's all I can say about your case. Everything that follows is
just a shot in the dark, and others may have better ideas. Also, look
here:
https://github.com/gitster/git/blob/master/Documentation/howto/recover-corrupted-blob-object.txt
You can remove the *.idx files, because they do not carry essential
information. Now try git fsck; git repack.
Try the reverse edit:
find .git -name '*.*' -exec sed -i 's/ /\t/g' {} \;
Remove .git/index; it can be reconstructed (of course, assuming you
started all this with a clean index; if not, you lose the staged changes).
-- Hannes
^ permalink raw reply
* Re: [PATCH] archive-tar: fix sanity check in config parsing
From: Jeff King @ 2013-01-14 12:44 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: git
In-Reply-To: <kd0evl$ac0$1@ger.gmane.org>
On Mon, Jan 14, 2013 at 09:17:57AM +0100, Joachim Schmitz wrote:
> >For the curious, the original version of the patch[1] read:
> >
> >+ if (prefixcmp(var, "tarfilter."))
> >+ return 0;
> >+ dot = strrchr(var, '.');
> >+ if (dot == var + 9)
> >+ return 0;
> >
> >and when I shortened the config section to "tar" in a re-roll of the
> >series, I missed the corresponding change to the offset.
>
> Wouldn't it then be better ti use strlen("tar") rather than a 3? Or
> at least a comment?
Then you are relying on the two strings being the same, rather than the
string and the length being the same. If you wanted to DRY it up, it
would look like:
diff --git a/archive-tar.c b/archive-tar.c
index d1cce46..a7c0690 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -332,15 +332,17 @@ static int tar_filter_config(const char *var, const char *value, void *data)
const char *type;
int namelen;
- if (prefixcmp(var, "tar."))
+#define SECTION "tar"
+ if (prefixcmp(var, SECTION "."))
return 0;
dot = strrchr(var, '.');
- if (dot == var + 9)
+ if (dot == var + strlen(SECTION))
return 0;
- name = var + 4;
+ name = var + strlen(SECTION) + 1;
namelen = dot - name;
type = dot + 1;
+#undef SECTION
ar = find_tar_filter(name, namelen);
if (!ar) {
(of course there are other variants where you do not use a macro, but
then you need to manually check for the "." after the prefixcmp call).
I dunno. It is technically more robust in that the offsets are computed,
but I think it is a little harder to read. Of course, I wrote the
original so I am probably not a good judge.
We could also potentially encapsulate it in a function. I think the diff
code has a very similar block.
-Peff
^ permalink raw reply related
* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Michael J Gruber @ 2013-01-14 13:07 UTC (permalink / raw)
To: Jardel Weyrich; +Cc: Sascha Cunz, Junio C Hamano, git
In-Reply-To: <CAN8TAOv0Cm8CgiJSweFtRzOqO78OtNKa4G+x7z6M5Bt+odUmiQ@mail.gmail.com>
Jardel Weyrich venit, vidit, dixit 12.01.2013 10:33:
> On Sat, Jan 12, 2013 at 6:44 AM, Sascha Cunz <sascha-ml@babbelbox.org> wrote:
>> Am Freitag, 11. Januar 2013, 23:10:36 schrieb Junio C Hamano:
>>> Jardel Weyrich <jweyrich@gmail.com> writes:
>>>> I believe `remote set-url --add --push` has a bug. Performed tests
>>>> with v1.8.0.1 and v1.8.1 (Mac OS X).
>>>>
>>>> Quoting the relevant part of the documentation:
>>>>> set-url
>>>>>
>>>>> Changes URL remote points to. Sets first URL remote points to
>>>>> matching regex <oldurl> (first URL if no <oldurl> is given) to
>>>>> <newurl>. If <oldurl> doesn’t match any URL, error occurs and
>>>>> nothing is changed.
>>>>>
>>>>> With --push, push URLs are manipulated instead of fetch URLs.
>>>>> With --add, instead of changing some URL, new URL is added.
>>>>> With --delete, instead of changing some URL, all URLs matching regex
>>>>> <url> are deleted. Trying to delete all non-push URLs is an error.>
>>>> Here are some steps to reproduce:
>>>>
>>>> 1. Show the remote URLs
>>>>
>>>> jweyrich@pharao:test_clone1 [* master]$ git remote -v
>>>> origin /Volumes/sandbox/test (fetch)
>>>> origin /Volumes/sandbox/test (push)
>>>>
>>>> 2. Add a new push URL for origin
>>>>
>>>> jweyrich@pharao:test_clone1 [* master]$ git remote set-url --add --push
>>>> origin \>
>>>> /Volumes/sandbox/test_clone2
>>>>
>>>> 3. Check what happened
>>>>
>>>> jweyrich@pharao:test_clone1 [* master]$ git remote -v
>>>> origin /Volumes/sandbox/test (fetch)
>>>> origin /Volumes/sandbox/test_clone2 (push)
>>>
>>> The original pushurl was replaced with the additional one, instead
>>> of being left and the new one getting added. That looks certainly
>>> wrong.
>>>
>>> However, the result of applying the attached patch (either to
>>> v1.7.12 or v1.8.1) still passes the test and I do not think it is
>>> doing anything differently from what you described above.
>>>
>>> What do you get from
>>>
>>> git config -l | grep '^remote\.origin'
>>>
>>> in steps 1. and 3. in your procedure? This question is trying to
>>> tell if your bug is in "git remote -v" or in "git remote set-url".
>>
>> I'm not sure, if there is a bug at all. According to man git-push:
>>
>> The <pushurl> is used for pushes only. It is optional and defaults to
>> <url>.
>> (From the section REMOTES -> Named remote in configuration file)
>>
>> the command:
>> git remote add foo git@foo-fetch.org/some.git
>>
>> will set "remote.foo.url" to "git@foo-fetch.org". Subsequently, fetch and push
>> will use git@foo-fetch.org as url.
>> Fetch will use this url, because "remote.foo.url" explicitly sets this. push
>> will use it in absence of a "remote.foo.pushurl".
>>
>> Now, we're adding a push-url:
>> git remote set-url --add --push foo git@foo-push.org/some.git
>>
>> Relevant parts of config are now looking like:
>> [remote "foo"]
>> url = git@foo-fetch.org/some.git
>> pushurl = git@foo-push.org/some.git
>>
>> Since, pushurl is now given explicitly, git push will use that one (and only
>> that one).
>>
>> If we add another push-url now,
>> git remote set-url --add --push foo git@foo-push-also.org/some.git
>>
>> the next git-push will push to foo-push.org and foo-push-also.org.
>>
>> Now, using --set-url --delete on both of these urls restores the original
>> state: only "remote.foo.url" is set; meaning implicitly pushurl defaults to
>> url again.
>>
>> To me this is exactly what Jardel was observing:
>>
>>> In step 2, Git replaced the original push URL instead of adding a new
>>> one. But it seems to happen only the first time I use `remote set-url
>>> --add --push`. Re-adding the original URL using the same command seems
>>> to work properly.
>>
>>> And FWIW, if I delete (with "set-url --delete") both URLs push, Git
>>> restores the original URL.
>>
>> Or am I missing something here?
>
> You're right. However, as I quoted earlier, the git-remote man-page states:
>
> set-url
> Changes URL remote points to. <suppressed>
> With --push, push URLs are manipulated instead of fetch URLs.
> With --add, instead of changing some URL, new URL is added.
>
> It explicitly mentions that it should **add a new URL**.
> So when I do `git remote set-url --add --push origin
> git://another/repo.git`, I expect git-push to use both the default
> push URL and the new one. Or am I misinterpreting the man-page?
>
>>
>> Might be that the "bug" actually is that the expectation was
>>
>> git remote add foo git@foo-fetch.org/some.git
>>
>> should have created a config like:
>>
>> [remote "foo"]
>> url = git@foo-fetch.org/some.git
>> pushurl = git@foo-fetch.org/some.git
>>
>> since that is what "git remote -v" reports.
>>
>> If that is the case, we might want to amend the output of 'git remote -v' with
>> the information that a pushurl is not explicitly given and thus defaults to
>> url.
>
> Correct. Adding a remote doesn't automatically generate a pushurl for it.
>
> To me, it seems that git-push checks for the existence of pushurl's in
> the config, and if it finds any, it ignores the defaul push URL during
> the actual push. In better words, it pushes only to pushurls, if it
> finds any, otherwise it pushes to the default URL.
>
> To comply with the statements in the git-remote man-page, git-remote
> should add a pushurl configuration containing the default push URL,
> along with the one passed to `set-url --add --push`. Or git-push
> should _not ignore_ the default URL in the presence of pushurls,
> effectively pushing to both. These are the solutions I can think of
> right now, supposing I'm correct about the cause(s).
>
>>
>> Sascha
All that "set-url --push --add" does is adding a remote.foo.pushurl
entry to the config. If there was none, there will be one after that.
If there is no pushurl entry, "push" takes the url entry instead. This
is the "default URL for push", but not a pushurl entry.
It seems to me that everything works as designed, and that the man page
talk about "push URLs" can be read in two ways, one of which is correct
(and which is obvious if you know the above, i.e. the "config
background") and one of which is incorrect (and which may be obvious if
you read just that man page paragraph).
Michael
^ permalink raw reply
* Re: [PATCH] rebase --preserve-merges keeps empty merge commits
From: Neil Horman @ 2013-01-14 14:02 UTC (permalink / raw)
To: Phil Hord; +Cc: git, phil.hord, Martin von Zweigbergk, Junio C Hamano
In-Reply-To: <1358023561-26773-1-git-send-email-hordp@cisco.com>
On Sat, Jan 12, 2013 at 03:46:01PM -0500, Phil Hord wrote:
> Since 90e1818f9a (git-rebase: add keep_empty flag, 2012-04-20)
> 'git rebase --preserve-merges' fails to preserve empty merge commits
> unless --keep-empty is also specified. Merge commits should be
> preserved in order to preserve the structure of the rebased graph,
> even if the merge commit does not introduce changes to the parent.
>
> Teach rebase not to drop merge commits only because they are empty.
>
> A special case which is not handled by this change is for a merge commit
> whose parents are now the same commit because all the previous different
> parents have been dropped as a result of this rebase or some previous
> operation.
> ---
> git-rebase--interactive.sh | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 44901d5..8ed7fcc 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -190,6 +190,11 @@ is_empty_commit() {
> test "$tree" = "$ptree"
> }
>
> +is_merge_commit()
> +{
> + git rev-parse --verify --quiet "$1"^2 >/dev/null 2>&1
> +}
> +
> # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
> # GIT_AUTHOR_DATE exported from the current environment.
> do_with_author () {
> @@ -874,7 +879,7 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \
> while read -r shortsha1 rest
> do
>
> - if test -z "$keep_empty" && is_empty_commit $shortsha1
> + if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
> then
> comment_out="# "
> else
> --
> 1.8.1.dirty
>
>
Seems reasonable
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH] rebase --preserve-merges keeps empty merge commits
From: Matthieu Moy @ 2013-01-14 14:12 UTC (permalink / raw)
To: Phil Hord
Cc: git, phil.hord, Neil Horman, Martin von Zweigbergk,
Junio C Hamano
In-Reply-To: <1358023561-26773-1-git-send-email-hordp@cisco.com>
Phil Hord <hordp@cisco.com> writes:
> Subject: [PATCH] rebase --preserve-merges keeps empty merge commits
I would rephrase it as
rebase --preserve-merges: keep empty merge commits
we usually give orders in commit messages, not state facts (it's not
clear from the existing subject line whether keeping merge commit is the
new behavior or a bug that the commit tries to fix).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] archive-tar: fix sanity check in config parsing
From: Jeff King @ 2013-01-14 14:58 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114124424.GA14129@sigill.intra.peff.net>
On Mon, Jan 14, 2013 at 04:44:24AM -0800, Jeff King wrote:
> > Wouldn't it then be better ti use strlen("tar") rather than a 3? Or
> > at least a comment?
> [...]
> We could also potentially encapsulate it in a function. I think the diff
> code has a very similar block.
Here's a series that does that, with a few other cleanups on top. The
diffstat actually ends up a few lines longer, but that is mostly because
of comments and function declarations. More importantly, though, the
call-sites are much easier to read.
Having written this series, though, I can't help but wonder if the world
would be a better place if config_fn_t looked more like:
typedef int (*config_fn_t)(const char *full_var,
const char *section,
const char *subsection,
const char *key,
const char *value,
void *data);
It's just as easy for the config parser to do this ahead of time, and by
handing off real C-strings (instead of ending up with a ptr/len pair for
the subsection), it makes the lives of the callbacks much easier (e.g.,
the final patch below contorts a bit to use string_list with the
subsection).
I can look into that, but here is the less invasive cleanup:
[1/6]: config: add helper function for parsing key names
[2/6]: archive-tar: use match_config_key when parsing config
[3/6]: convert some config callbacks to match_config_key
[4/6]: userdiff: drop parse_driver function
[5/6]: submodule: use match_config_key when parsing config
[6/6]: submodule: simplify memory handling in config parsing
-Peff
^ permalink raw reply
* [PATCH 1/6] config: add helper function for parsing key names
From: Jeff King @ 2013-01-14 15:00 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114145845.GA16497@sigill.intra.peff.net>
The config callback functions get keys of the general form:
section.subsection.key
(where the subsection may be contain arbitrary data, or may
be missing). For matching keys without subsections, it is
simple enough to call "strcmp". Matching keys with
subsections is a little more complicated, and each callback
does it in an ad-hoc way, usually involving error-prone
pointer arithmetic.
Let's provide a helper that keeps the pointer arithmetic all
in one place.
Signed-off-by: Jeff King <peff@peff.net>
---
No users yet; they come in future patches.
cache.h | 15 +++++++++++++++
config.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/cache.h b/cache.h
index c257953..14003b8 100644
--- a/cache.h
+++ b/cache.h
@@ -1164,6 +1164,21 @@ extern int git_config_include(const char *name, const char *value, void *data);
#define CONFIG_INCLUDE_INIT { 0 }
extern int git_config_include(const char *name, const char *value, void *data);
+/*
+ * Match and parse a config key of the form:
+ *
+ * section.(subsection.)?key
+ *
+ * (i.e., what gets handed to a config_fn_t). The caller provides the section;
+ * we return -1 if it does not match, 0 otherwise. The subsection and key
+ * out-parameters are filled by the function (and subsection is NULL if it is
+ * missing).
+ */
+extern int match_config_key(const char *var,
+ const char *section,
+ const char **subsection, int *subsection_len,
+ const char **key);
+
extern int committer_ident_sufficiently_given(void);
extern int author_ident_sufficiently_given(void);
diff --git a/config.c b/config.c
index 7b444b6..18d9c0e 100644
--- a/config.c
+++ b/config.c
@@ -1667,3 +1667,36 @@ int config_error_nonbool(const char *var)
{
return error("Missing value for '%s'", var);
}
+
+int match_config_key(const char *var,
+ const char *section,
+ const char **subsection, int *subsection_len,
+ const char **key)
+{
+ int section_len = strlen(section);
+ const char *dot;
+
+ /* Does it start with "section." ? */
+ if (prefixcmp(var, section) || var[section_len] != '.')
+ return -1;
+
+ /*
+ * Find the key; we don't know yet if we have a subsection, but we must
+ * parse backwards from the end, since the subsection may have dots in
+ * it, too.
+ */
+ dot = strrchr(var, '.');
+ *key = dot + 1;
+
+ /* Did we have a subsection at all? */
+ if (dot == var + section_len) {
+ *subsection = NULL;
+ *subsection_len = 0;
+ }
+ else {
+ *subsection = var + section_len + 1;
+ *subsection_len = dot - *subsection;
+ }
+
+ return 0;
+}
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* [PATCH 2/6] archive-tar: use match_config_key when parsing config
From: Jeff King @ 2013-01-14 15:02 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114145845.GA16497@sigill.intra.peff.net>
This is fewer lines of code, but more importantly, fixes a
bogus pointer offset. We are looking for "tar." in the
section, but later assume that the dot we found is at offset
9, not 3. This is a holdover from an earlier iteration of
767cf45 which called the section "tarfilter".
As a result, we could erroneously reject some filters with
dots in their name, as well as read uninitialized memory.
Reported by (and test by) René Scharfe.
Signed-off-by: Jeff King <peff@peff.net>
---
This obviously replaces René's patch. It might make more sense to just
put his patch onto maint, and build this on top; then this patch can get
squashed into the next one (which just updates the uninteresting
callsites).
archive-tar.c | 10 +---------
t/t5000-tar-tree.sh | 3 ++-
2 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/archive-tar.c b/archive-tar.c
index 0ba3f25..68dbe59 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -325,20 +325,12 @@ static int tar_filter_config(const char *var, const char *value, void *data)
static int tar_filter_config(const char *var, const char *value, void *data)
{
struct archiver *ar;
- const char *dot;
const char *name;
const char *type;
int namelen;
- if (prefixcmp(var, "tar."))
+ if (match_config_key(var, "tar", &name, &namelen, &type) < 0 || !name)
return 0;
- dot = strrchr(var, '.');
- if (dot == var + 9)
- return 0;
-
- name = var + 4;
- namelen = dot - name;
- type = dot + 1;
ar = find_tar_filter(name, namelen);
if (!ar) {
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index ecf00ed..517ae04 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -283,7 +283,8 @@ test_expect_success 'setup tar filters' '
test_expect_success 'setup tar filters' '
git config tar.tar.foo.command "tr ab ba" &&
git config tar.bar.command "tr ab ba" &&
- git config tar.bar.remote true
+ git config tar.bar.remote true &&
+ git config tar.invalid baz
'
test_expect_success 'archive --list mentions user filter' '
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* [PATCH 3/6] convert some config callbacks to match_config_key
From: Jeff King @ 2013-01-14 15:03 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114145845.GA16497@sigill.intra.peff.net>
This is easier to read and avoids magic offset constants
which need to be in sync with the section-name we provide.
Signed-off-by: Jeff King <peff@peff.net>
---
convert.c | 6 +-----
ll-merge.c | 6 +-----
userdiff.c | 13 +++----------
3 files changed, 5 insertions(+), 20 deletions(-)
diff --git a/convert.c b/convert.c
index 6602155..e3ecb30 100644
--- a/convert.c
+++ b/convert.c
@@ -465,10 +465,8 @@ static int read_convert_config(const char *var, const char *value, void *cb)
* External conversion drivers are configured using
* "filter.<name>.variable".
*/
- if (prefixcmp(var, "filter.") || (ep = strrchr(var, '.')) == var + 6)
+ if (match_config_key(var, "filter", &name, &namelen, &ep) < 0 || !name)
return 0;
- name = var + 7;
- namelen = ep - name;
for (drv = user_convert; drv; drv = drv->next)
if (!strncmp(drv->name, name, namelen) && !drv->name[namelen])
break;
@@ -479,8 +477,6 @@ static int read_convert_config(const char *var, const char *value, void *cb)
user_convert_tail = &(drv->next);
}
- ep++;
-
/*
* filter.<name>.smudge and filter.<name>.clean specifies
* the command line:
diff --git a/ll-merge.c b/ll-merge.c
index acea33b..d4c4ff6 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -236,15 +236,13 @@ static int read_merge_config(const char *var, const char *value, void *cb)
* especially, we do not want to look at variables such as
* "merge.summary", "merge.tool", and "merge.verbosity".
*/
- if (prefixcmp(var, "merge.") || (ep = strrchr(var, '.')) == var + 5)
+ if (match_config_key(var, "merge", &name, &namelen, &ep) < 0 || !name)
return 0;
/*
* Find existing one as we might be processing merge.<name>.var2
* after seeing merge.<name>.var1.
*/
- name = var + 6;
- namelen = ep - name;
for (fn = ll_user_merge; fn; fn = fn->next)
if (!strncmp(fn->name, name, namelen) && !fn->name[namelen])
break;
@@ -256,8 +254,6 @@ static int read_merge_config(const char *var, const char *value, void *cb)
ll_user_merge_tail = &(fn->next);
}
- ep++;
-
if (!strcmp("name", ep)) {
if (!value)
return error("%s: lacks value", var);
diff --git a/userdiff.c b/userdiff.c
index ed958ef..1a6a0fa 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -188,20 +188,13 @@ static struct userdiff_driver *parse_driver(const char *var,
const char *value, const char *type)
{
struct userdiff_driver *drv;
- const char *dot;
- const char *name;
+ const char *name, *key;
int namelen;
- if (prefixcmp(var, "diff."))
- return NULL;
- dot = strrchr(var, '.');
- if (dot == var + 4)
- return NULL;
- if (strcmp(type, dot+1))
+ if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
+ strcmp(type, key))
return NULL;
- name = var + 5;
- namelen = dot - name;
drv = userdiff_find_by_namelen(name, namelen);
if (!drv) {
ALLOC_GROW(drivers, ndrivers+1, drivers_alloc);
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* [PATCH 4/6] userdiff: drop parse_driver function
From: Jeff King @ 2013-01-14 15:04 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114145845.GA16497@sigill.intra.peff.net>
When we parse userdiff config, we generally assume that
diff.name.key
will affect the "key" value of the "name" driver. However,
without checking the key, we conflict with the ancient
"diff.color.*" namespace. The current code is careful not to
even create a driver struct if we do not see a key that is
known by the diff-driver code.
However, this carefulness is unnecessary; the default driver
with no keys set behaves exactly the same as having no
driver at all. We can simply set up the driver struct as
soon as we see we have a config key that looks like a
driver. This makes the code a bit more readable.
Signed-off-by: Jeff King <peff@peff.net>
---
This is not strictly related to the series, but I noticed it as a
cleanup while doing the previous patch.
userdiff.c | 50 +++++++++++++++++++++-----------------------------
1 file changed, 21 insertions(+), 29 deletions(-)
diff --git a/userdiff.c b/userdiff.c
index 1a6a0fa..c6cdec4 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -184,28 +184,6 @@ static struct userdiff_driver *userdiff_find_by_namelen(const char *k, int len)
return NULL;
}
-static struct userdiff_driver *parse_driver(const char *var,
- const char *value, const char *type)
-{
- struct userdiff_driver *drv;
- const char *name, *key;
- int namelen;
-
- if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
- strcmp(type, key))
- return NULL;
-
- drv = userdiff_find_by_namelen(name, namelen);
- if (!drv) {
- ALLOC_GROW(drivers, ndrivers+1, drivers_alloc);
- drv = &drivers[ndrivers++];
- memset(drv, 0, sizeof(*drv));
- drv->name = xmemdupz(name, namelen);
- drv->binary = -1;
- }
- return drv;
-}
-
static int parse_funcname(struct userdiff_funcname *f, const char *k,
const char *v, int cflags)
{
@@ -233,20 +211,34 @@ int userdiff_config(const char *k, const char *v)
int userdiff_config(const char *k, const char *v)
{
struct userdiff_driver *drv;
+ const char *name, *type;
+ int namelen;
+
+ if (match_config_key(k, "diff", &name, &namelen, &type) || !name)
+ return 0;
+
+ drv = userdiff_find_by_namelen(name, namelen);
+ if (!drv) {
+ ALLOC_GROW(drivers, ndrivers+1, drivers_alloc);
+ drv = &drivers[ndrivers++];
+ memset(drv, 0, sizeof(*drv));
+ drv->name = xmemdupz(name, namelen);
+ drv->binary = -1;
+ }
- if ((drv = parse_driver(k, v, "funcname")))
+ if (!strcmp(type, "funcname"))
return parse_funcname(&drv->funcname, k, v, 0);
- if ((drv = parse_driver(k, v, "xfuncname")))
+ if (!strcmp(type, "xfuncname"))
return parse_funcname(&drv->funcname, k, v, REG_EXTENDED);
- if ((drv = parse_driver(k, v, "binary")))
+ if (!strcmp(type, "binary"))
return parse_tristate(&drv->binary, k, v);
- if ((drv = parse_driver(k, v, "command")))
+ if (!strcmp(type, "command"))
return git_config_string(&drv->external, k, v);
- if ((drv = parse_driver(k, v, "textconv")))
+ if (!strcmp(type, "textconv"))
return git_config_string(&drv->textconv, k, v);
- if ((drv = parse_driver(k, v, "cachetextconv")))
+ if (!strcmp(type, "cachetextconv"))
return parse_bool(&drv->textconv_want_cache, k, v);
- if ((drv = parse_driver(k, v, "wordregex")))
+ if (!strcmp(type, "wordregex"))
return git_config_string(&drv->word_regex, k, v);
return 0;
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* [PATCH 5/6] submodule: use match_config_key when parsing config
From: Jeff King @ 2013-01-14 15:04 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114145845.GA16497@sigill.intra.peff.net>
This makes the code a lot simpler to read by dropping a
whole bunch of constant offsets.
As a bonus, it means we also feed the whole config variable
name to our error functions:
[before]
$ git -c submodule.foo.fetchrecursesubmodules=bogus checkout
fatal: bad foo.fetchrecursesubmodules argument: bogus
[after]
fatal: bad submodule.foo.fetchrecursesubmodules argument: bogus
Signed-off-by: Jeff King <peff@peff.net>
---
submodule.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/submodule.c b/submodule.c
index 2f55436..4361207 100644
--- a/submodule.c
+++ b/submodule.c
@@ -126,15 +126,17 @@ int parse_submodule_config_option(const char *var, const char *value)
int parse_submodule_config_option(const char *var, const char *value)
{
- int len;
struct string_list_item *config;
struct strbuf submodname = STRBUF_INIT;
+ const char *name, *key;
+ int namelen;
- var += 10; /* Skip "submodule." */
+ if (match_config_key(var, "submodule", &name, &namelen, &key) < 0 ||
+ !name)
+ return 0;
- len = strlen(var);
- if ((len > 5) && !strcmp(var + len - 5, ".path")) {
- strbuf_add(&submodname, var, len - 5);
+ if (!strcmp(key, "path")) {
+ strbuf_add(&submodname, name, namelen);
config = unsorted_string_list_lookup(&config_name_for_path, value);
if (config)
free(config->util);
@@ -142,22 +144,22 @@ int parse_submodule_config_option(const char *var, const char *value)
config = string_list_append(&config_name_for_path, xstrdup(value));
config->util = strbuf_detach(&submodname, NULL);
strbuf_release(&submodname);
- } else if ((len > 23) && !strcmp(var + len - 23, ".fetchrecursesubmodules")) {
- strbuf_add(&submodname, var, len - 23);
+ } else if (!strcmp(key, "fetchrecursesubmodules")) {
+ strbuf_add(&submodname, name, namelen);
config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);
if (!config)
config = string_list_append(&config_fetch_recurse_submodules_for_name,
strbuf_detach(&submodname, NULL));
config->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);
strbuf_release(&submodname);
- } else if ((len > 7) && !strcmp(var + len - 7, ".ignore")) {
+ } else if (!strcmp(key, "ignore")) {
if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
strcmp(value, "all") && strcmp(value, "none")) {
warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
return 0;
}
- strbuf_add(&submodname, var, len - 7);
+ strbuf_add(&submodname, name, namelen);
config = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);
if (config)
free(config->util);
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* [PATCH 6/6] submodule: simplify memory handling in config parsing
From: Jeff King @ 2013-01-14 15:07 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114145845.GA16497@sigill.intra.peff.net>
We keep a strbuf for the name of the submodule, even though
we only ever add one buffer (which we know the length of) to
it. Let's just use xmemdupz instead, which is slightly more
efficient and makes it easier to follow what is going on.
Unfortunately, we still end up having to deal with some
memory ownership issues in some code branches, as we have to
allocate the string in order to do a string list lookup, and
then only sometimes want to hand ownership of that string
over to the string_list. Still, making that explicit in the
code (as opposed to sometimes detaching the strbuf, and then
always releasing it) makes it a little more obvious what is
going on.
Signed-off-by: Jeff King <peff@peff.net>
---
I'm undecided on this one. I think the result is easier to follow, but
others might find the original easier. This would be a lot more
readable if the config parser gave us a broken-out representation with
real C strings. Then we could pass the subsection straight to the
string_list functions for lookup, and using the "dup" flag of string
list, avoid even having to deal with memory duplication at all.
submodule.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/submodule.c b/submodule.c
index 4361207..23a8490 100644
--- a/submodule.c
+++ b/submodule.c
@@ -127,7 +127,6 @@ int parse_submodule_config_option(const char *var, const char *value)
int parse_submodule_config_option(const char *var, const char *value)
{
struct string_list_item *config;
- struct strbuf submodname = STRBUF_INIT;
const char *name, *key;
int namelen;
@@ -136,37 +135,36 @@ int parse_submodule_config_option(const char *var, const char *value)
return 0;
if (!strcmp(key, "path")) {
- strbuf_add(&submodname, name, namelen);
config = unsorted_string_list_lookup(&config_name_for_path, value);
if (config)
free(config->util);
else
config = string_list_append(&config_name_for_path, xstrdup(value));
- config->util = strbuf_detach(&submodname, NULL);
- strbuf_release(&submodname);
+ config->util = xmemdupz(name, namelen);
} else if (!strcmp(key, "fetchrecursesubmodules")) {
- strbuf_add(&submodname, name, namelen);
- config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);
+ char *name_cstr = xmemdupz(name, namelen);
+ config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, name_cstr);
if (!config)
- config = string_list_append(&config_fetch_recurse_submodules_for_name,
- strbuf_detach(&submodname, NULL));
+ config = string_list_append(&config_fetch_recurse_submodules_for_name, name_cstr);
+ else
+ free(name_cstr);
config->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);
- strbuf_release(&submodname);
} else if (!strcmp(key, "ignore")) {
+ char *name_cstr;
+
if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
strcmp(value, "all") && strcmp(value, "none")) {
warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
return 0;
}
- strbuf_add(&submodname, name, namelen);
- config = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);
- if (config)
+ name_cstr = xmemdupz(name, namelen);
+ config = unsorted_string_list_lookup(&config_ignore_for_name, name_cstr);
+ if (config) {
free(config->util);
- else
- config = string_list_append(&config_ignore_for_name,
- strbuf_detach(&submodname, NULL));
- strbuf_release(&submodname);
+ free(name_cstr);
+ } else
+ config = string_list_append(&config_ignore_for_name, name_cstr);
config->util = xstrdup(value);
return 0;
}
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* Re: git list files
From: Jeff King @ 2013-01-14 15:28 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Стойчо Слепцов,
git, Jakub Narębski, Matthieu Moy
In-Reply-To: <20130114070832.GA4820@elie.Belkin>
On Sun, Jan 13, 2013 at 11:08:32PM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > As far as I recall, that script works. However, I have a pure-C
> > blame-tree implementation that is much faster, which may also be of
> > interest. I need to clean up and put a few finishing touches on it to
> > send it to the list, but it has been in production at GitHub for a few
> > months. You can find it here:
> >
> > git://github.com/peff/git jk/blame-tree
>
> Oh, neat. Would that make sense as an item in
> <https://git.wiki.kernel.org/index.php/Interfaces,_frontends,_and_tools>?
I'd rather finish cleaning it up and actually get it merged. It's on my
todo list.
-Peff
^ permalink raw reply
* [PATCH] pretty: use prefixcmp instead of memcmp on NUL-terminated strings
From: René Scharfe @ 2013-01-14 16:34 UTC (permalink / raw)
To: git discussion list; +Cc: Junio C Hamano
This conversion avoids the need for magic string length numbers in the
code. And unlike memcmp(), prefixcmp() is careful to not run over the
end of a string.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
pretty.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/pretty.c b/pretty.c
index 92c839f..01795de 100644
--- a/pretty.c
+++ b/pretty.c
@@ -966,7 +966,7 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
if (!end)
return 0;
- if (!memcmp(begin, "auto,", 5)) {
+ if (!prefixcmp(begin, "auto,")) {
if (!want_color(c->pretty_ctx->color))
return end - placeholder + 1;
begin += 5;
@@ -1301,7 +1301,7 @@ static void pp_header(const struct pretty_print_context *pp,
continue;
}
- if (!memcmp(line, "parent ", 7)) {
+ if (!prefixcmp(line, "parent ")) {
if (linelen != 48)
die("bad parent line in commit");
continue;
@@ -1325,11 +1325,11 @@ static void pp_header(const struct pretty_print_context *pp,
* FULL shows both authors but not dates.
* FULLER shows both authors and dates.
*/
- if (!memcmp(line, "author ", 7)) {
+ if (!prefixcmp(line, "author ")) {
strbuf_grow(sb, linelen + 80);
pp_user_info(pp, "Author", sb, line + 7, encoding);
}
- if (!memcmp(line, "committer ", 10) &&
+ if (!prefixcmp(line, "committer ") &&
(pp->fmt == CMIT_FMT_FULL || pp->fmt == CMIT_FMT_FULLER)) {
strbuf_grow(sb, linelen + 80);
pp_user_info(pp, "Commit", sb, line + 10, encoding);
--
1.8.0
^ permalink raw reply related
* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Jonathan Nieder @ 2013-01-14 16:41 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Jardel Weyrich, Sascha Cunz, Junio C Hamano, git
In-Reply-To: <50F40316.7010308@drmicha.warpmail.net>
Michael J Gruber wrote:
> All that "set-url --push --add" does is adding a remote.foo.pushurl
> entry to the config. If there was none, there will be one after that.
>
> If there is no pushurl entry, "push" takes the url entry instead. This
> is the "default URL for push", but not a pushurl entry.
That is how it is implemented, but it is hard for me with a straight
face to say that is what most users expect.
Wouldn't the least confusing thing be to just error out for "set-url
--push --add" when there is no existing pushurl? That way, the
operator can use plain "set-url --push" to clarify whether whether he
meant to include the pull URLs in the new pushurl set.
My two cents,
Jonathan
^ permalink raw reply
* Re: [PATCH 3/6] convert some config callbacks to match_config_key
From: Jonathan Nieder @ 2013-01-14 16:55 UTC (permalink / raw)
To: Jeff King; +Cc: Joachim Schmitz, René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114150322.GC16828@sigill.intra.peff.net>
Jeff King wrote:
> --- a/convert.c
> +++ b/convert.c
> @@ -465,10 +465,8 @@ static int read_convert_config(const char *var, const char *value, void *cb)
> * External conversion drivers are configured using
> * "filter.<name>.variable".
> */
> - if (prefixcmp(var, "filter.") || (ep = strrchr(var, '.')) == var + 6)
> + if (match_config_key(var, "filter", &name, &namelen, &ep) < 0 || !name)
> return 0;
Hm, I actually find the preimage more readable here.
I like the idea of having a function to do this, though. Here are a
couple of ideas for making the meaning obvious again for people like
me:
Rename match_config_key() to something like parse_config_key()?
match_ makes it sound like its main purpose is to match it against a
pattern, but really it is more about decomposing into constituent
parts.
Rename ep to something like 'key' or 'filtertype'? Without the
explicit string processing, it is not obvious what ep is the end of.
[...]
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -188,20 +188,13 @@ static struct userdiff_driver *parse_driver(const char *var,
> const char *value, const char *type)
> {
> struct userdiff_driver *drv;
> - const char *dot;
> - const char *name;
> + const char *name, *key;
> int namelen;
>
> - if (prefixcmp(var, "diff."))
> - return NULL;
> - dot = strrchr(var, '.');
> - if (dot == var + 4)
> - return NULL;
> - if (strcmp(type, dot+1))
> + if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
> + strcmp(type, key))
> return NULL;
>
> - name = var + 5;
> - namelen = dot - name;
> drv = userdiff_find_by_namelen(name, namelen);
What happens in the !name case? (Honest question --- I haven't checked.)
Generally I like the cleanup. Thanks for tasteful patch.
Jonathan
^ permalink raw reply
* Re: [PATCH 3/6] convert some config callbacks to match_config_key
From: Jeff King @ 2013-01-14 17:06 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Joachim Schmitz, René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114165527.GB3121@elie.Belkin>
On Mon, Jan 14, 2013 at 08:55:28AM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > --- a/convert.c
> > +++ b/convert.c
> > @@ -465,10 +465,8 @@ static int read_convert_config(const char *var, const char *value, void *cb)
> > * External conversion drivers are configured using
> > * "filter.<name>.variable".
> > */
> > - if (prefixcmp(var, "filter.") || (ep = strrchr(var, '.')) == var + 6)
> > + if (match_config_key(var, "filter", &name, &namelen, &ep) < 0 || !name)
> > return 0;
>
> Hm, I actually find the preimage more readable here.
The big thing to me is getting rid of the manual "6" there, which is bad
for maintainability (it must match the length of "filter", and there is
no compile-time verification).
> Rename match_config_key() to something like parse_config_key()?
> match_ makes it sound like its main purpose is to match it against a
> pattern, but really it is more about decomposing into constituent
> parts.
There is already a git_config_parse_key, but it does something else. I
wanted to avoid confusion there. And I was trying to indicate that this
was not just about parsing, but also about matching the section.
Basically I was trying to encapsulate the current technique and have as
little code change as possible. But maybe:
struct config_key k;
parse_config_key(&k, var);
if (strcmp(k.section, "filter") || k.subsection))
return 0;
would be a better start (or having git_config do the first two lines
itself before triggering the callback).
> Rename ep to something like 'key' or 'filtertype'? Without the
> explicit string processing, it is not obvious what ep is the end of.
Ah, so that is what "ep" stands for. I was thinking it is a terrible
variable name, but I was trying to keep the patch minimal.
> > - if (prefixcmp(var, "diff."))
> > - return NULL;
> > - dot = strrchr(var, '.');
> > - if (dot == var + 4)
> > - return NULL;
> > - if (strcmp(type, dot+1))
> > + if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
> > + strcmp(type, key))
> > return NULL;
> >
> > - name = var + 5;
> > - namelen = dot - name;
> > drv = userdiff_find_by_namelen(name, namelen);
>
> What happens in the !name case? (Honest question --- I haven't checked.)
Segfault, I expect. Thanks for catching.
I actually wrote this correctly once, coupled with patch 4, but screwed
it up when teasing it apart into two patches. It should be:
if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
!name ||
strcmp(type, key))
return NULL;
Patch 4 replaces this with correct code (as it moves it into
userdiff_config).
-Peff
^ permalink raw reply
* Re: [PATCH] rebase --preserve-merges keeps empty merge commits
From: Junio C Hamano @ 2013-01-14 17:15 UTC (permalink / raw)
To: Matthieu Moy
Cc: Phil Hord, git, phil.hord, Neil Horman, Martin von Zweigbergk
In-Reply-To: <vpqpq17zwdl.fsf@grenoble-inp.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Phil Hord <hordp@cisco.com> writes:
>
>> Subject: [PATCH] rebase --preserve-merges keeps empty merge commits
>
> I would rephrase it as
>
> rebase --preserve-merges: keep empty merge commits
>
> we usually give orders in commit messages, not state facts (it's not
> clear from the existing subject line whether keeping merge commit is the
> new behavior or a bug that the commit tries to fix).
Thanks for giving a concise rationale on our use of imperative mood.
Phil, I think you meant to and forgot to sign-off; here is what I'll
queue.
Thanks.
-- >8 --
From: Phil Hord <hordp@cisco.com>
Date: Sat, 12 Jan 2013 15:46:01 -0500
Subject: [PATCH] rebase --preserve-merges: keep all merge commits including empty ones
Since 90e1818f9a (git-rebase: add keep_empty flag, 2012-04-20)
'git rebase --preserve-merges' fails to preserve empty merge commits
unless --keep-empty is also specified. Merge commits should be
preserved in order to preserve the structure of the rebased graph,
even if the merge commit does not introduce changes to the parent.
Teach rebase not to drop merge commits only because they are empty.
A special case which is not handled by this change is for a merge commit
whose parents are now the same commit because all the previous different
parents have been dropped as a result of this rebase or some previous
operation.
Signed-off-by: Phil Hord <hordp@cisco.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-rebase--interactive.sh | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0c19b7c..2fed92f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -175,6 +175,11 @@ is_empty_commit() {
test "$tree" = "$ptree"
}
+is_merge_commit()
+{
+ git rev-parse --verify --quiet "$1"^2 >/dev/null 2>&1
+}
+
# Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
# GIT_AUTHOR_DATE exported from the current environment.
do_with_author () {
@@ -796,7 +801,7 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \
while read -r shortsha1 rest
do
- if test -z "$keep_empty" && is_empty_commit $shortsha1
+ if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
then
comment_out="# "
else
--
1.8.1.1.338.g126d652
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox