Re: emacs buffer names
Duy Nguyen writes:
> The --daemon part is probably not worth mentioning because I always
> have one emacs window open. The problem is switch-buffer based on file
> name can be confusing ("git.c" and "git.c<2>", which belongs to which
> checkout?). I ended up modifying files in the wrong checkout all the
> time.
(setq uniquify-buffer-name-style (quote post-forward-angle-brackets) nil
(uniquify))
This puts enough of the directory name in the buffer name to make the
buffer names unique; very helpful!
--
-- Stephe
--
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: socket_perror() "bug"?
On Mon, Mar 31, 2014 at 5:50 PM, Junio C Hamano wrote:
> Thiago Farina writes:
>
>> In imap-send.c:socket_perror() we pass |func| as a parameter, which I
>> think it is the name of the function that "called" socket_perror, or
>> the name of the function which generated an error.
>>
>> But at line 184 and 187 it always assume it was SSL_connect.
>>
>> Should we instead call perror() and ssl_socket_error() with func?
>
> Looks that way to me, at least from a cursory look.
Would you accept such a patch?
diff --git a/imap-send.c b/imap-send.c
index 0bc6f7f..bb04768 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -181,10 +181,10 @@ static void socket_perror(const char *func,
struct imap_socket *sock, int ret)
case SSL_ERROR_NONE:
break;
case SSL_ERROR_SYSCALL:
- perror("SSL_connect");
+ perror(func);
break;
default:
- ssl_socket_perror("SSL_connect");
+ ssl_socket_perror(func);
break;
}
} else
--
Thiago Farina
--
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
git-rebase-todo gets popped prematurely
During a 'git rebase --continue', I got an error about having left a file in place which the next commit intended to add as new. Stupid me. So I rm'ed the file and tried again. This time, git rebase --continue succeeded. But it accidentally left out the next commit in my rebase-todo. I looked in the code and it seems that when the "pick" returns an error, rebase--interactive stops and lets the user clean up. But it assumes the index already tracks a conflicted merge, and so it removes the commit from the todo list. In this case, however, the conflicted merge was avoided by detecting it in advance. The result is that the "would be overwritten" conflict evicts the entire commit from the rebase action. I think the code needs to detect the difference between "merge failed; conflicted index" and "merge failed; no change". I think I can do this with 'git-status -s -uno', maybe. But I haven't tried it yet and it feels like I'm missing a case or two also. I tried to bisect this to some specific change, but it fails the same way as far back as 1.6.5. Test script follows in case anyone has a better idea how to approach this and wants to understand it better. #!/bin/sh set -x git --version rm -rf baz git init baz && cd baz echo initial>initial && git add initial && git commit -minitial echo foo>foo && git add foo && git commit -mfoo echo bar>bar && git add bar && git commit -mbar git log --oneline GIT_EDITOR='sed -i -e s/^pick/edit/ -e /^#/d' git rebase -i HEAD^^ touch bar git rebase --continue rm bar git rebase --continue git log --oneline And the tail of the output (note the missing "bar" commit even though "Successfully rebased"): + git log --oneline fcc9b6e bar 8121f15 foo a521fa1 initial + GIT_EDITOR='sed -i -e s/^pick/edit/ -e /^#/d' + git rebase -i 'HEAD^^' Stopped at 8121f1593ea5c66dc7e9af7719100c1fcf4ab721... foo You can amend the commit now, with git commit --amend Once you are satisfied with your changes, run git rebase --continue + touch bar + git rebase --continue error: The following untracked working tree files would be overwritten by merge: bar Please move or remove them before you can merge. Aborting Could not apply fcc9b6ef2941e870f88362edbe0f1078cebb20e5... bar + rm bar + git rebase --continue Successfully rebased and updated refs/heads/master. + git log --oneline 8121f15 foo a521fa1 initial -- 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
[RFC] git submodule split
Hello,
I needed to convert a subdirectory of a repo to a submodule and have the
histories of both repos linked together. I found that this was discussed
few years back [1], but the code seemed quite complicated and was not
merged.
[1]:
http://git.661346.n2.nabble.com/RFC-What-s-the-best-UI-for-git-submodule-split-tp2318127.html
Now, the situation is better, because git subtree can already do most of
the work. Below is a script that I used to split a submodule from my
repo. It basically consist of a call to 'git subtree split' followed by
'git filter-branch' to link the histories together.
I'd like to get some initial feedback on it before attempting to
integrate it with git sources (i.e. writing tests and doc). What do you
think?
Thanks,
-Michal
#!/bin/sh
set -e
. git-sh-setup
url=$1
dir=$2
test -d "$dir" || die "$dir is not a directory"
# Create subtree corresponding to the directory
subtree=$(git subtree split --prefix="$dir")
subtree_tag=tmp/submodule-split-$$
git tag $subtree_tag $subtree
superproject=$PWD
export subtree subtree_tag superproject
# Replace the directory with submodule reference in the whole history
git filter-branch -f --index-filter "
set -e
# Check whether the $dir exists in this commit
if git ls-files --error-unmatch '$dir' > /dev/null 2>&1; then
# Find subtree commit corresponding to the commit in the
# superproject (this could be made faster by not running git log
# for every commit)
subcommit=\$(git log --format='%T %H' $subtree |
grep ^\$(git ls-tree \$GIT_COMMIT -- '$dir'|awk '{print \$3}') |
awk '{print \$2}')
# filter-branch runs the filter in an empty work-tree - create the
# future submodule in it so that the 'git submodule add' below
# does not try to clone it.
if ! test -d '$dir'; then
mkdir -p '$dir'
( cd '$dir' && clear_local_git_env && git init --quiet && git pull
$superproject $subtree_tag )
fi
# Remove all files under $dir from index so that the 'git
# submodule add' below does not complain.
git ls-files '$dir'|git update-index --force-remove --stdin
# Add the submodule - the goal here is to create/update .gitmodules
git submodule add $url '$dir'
# Update the submodule commit hash to the correct value
echo \"16 \$subcommit $dir\"|git update-index --index-info
fi
"
# Replace the directory in the working tree with the submodule
( cd "$dir" && find -mindepth 1 -delete && git init && git pull $superproject
$subtree_tag )
# Clean up
git tag --delete $subtree_tag
--
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's cooking in git.git (Mar 2014, #08; Mon, 31)
On Mon, Mar 31, 2014 at 05:29:03PM -0700, Junio C Hamano wrote: > * hv/submodule-ignore-fix (2013-12-06) 4 commits > - disable complete ignorance of submodules for index <-> HEAD diff > - always show committed submodules in summary after commit > - teach add -f option for ignored submodules > - fix 'git add' to skip submodules configured as ignored > > Teach "git add" to be consistent with "git status" when changes to > submodules are set to be ignored, to avoid surprises after checking > with "git status" to see there isn't any change to be further added > and then see that "git add ." adds changes to them. > > I think a reroll is coming, so this may need to be replaced, but I > needed some practice with heavy conflict resolution. It conflicts > with two changes to "git add" that have been scheduled for Git 2.0 > quite badly, and I think I got the resolution right this time. > > Waiting for a reroll. Since Ronald and Jens picked up this topic[1], I think you can discard my series. Cheers Heiko [1] http://thread.gmane.org/gmane.comp.version-control.git/245341 -- 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.1] commit: add --ignore-submodules[=] parameter
On 2. 4. 2014 20:53, Jens Lehmann wrote: > Am 01.04.2014 23:59, schrieb Ronald Weiss: >> On 1. 4. 2014 22:23, Jens Lehmann wrote: >>> Am 01.04.2014 01:35, schrieb Ronald Weiss: On 1. 4. 2014 0:50, Ronald Weiss wrote: > On 31. 3. 2014 23:47, Ronald Weiss wrote: >> On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann >> wrote: >>> As Junio mentioned it would be great if you could teach the add >>> command also honor the --ignore-submodule command line option in >>> a companion patch. In the course of doing so you'll easily see if >>> I was right or not, then please just order them in the most logical >>> way. >> >> Well, if You (or Junio) really don't want my patch without another one >> for git add, I may try to do it. However, git add does not even honor >> the submodules' ignore setting from .gitmodules (just tested with git >> 1.9.1: "git add -u" doesn't honor it, while "git commit -a" does). So >> teaching git add the --ignore-submodules switch in current state >> doesn't seem right to me. You might propose to add also support for >> the ignore setting, to make "add -u" and "commit -a" more consistent. >> That seems like a good idea, but the effort needed is getting bigger, > > Well, now I actually looked at it, and it was pretty easy after all. > The changes below seem to enable support for both ignore setting in > .gitmodules, and also --ignore-submodules switch, for git add, on top > of my patch for commit. There is a catch. With the changes below, submodules are ignored by add even if explitely named on command line (eg. "git add x" does nothing if x is submodule with new commits, but with ignore=all in .gitmodules). That doesn't seem right. Any ideas, what to do about that? When exactly should such submodule be actually ignored? >>> >>> Me thinks git add should require the '-f' option to add an ignored >>> submodule (just like it does for files) unless the user uses the >>> '--ignore-submodules=none' option. And if neither of these are given >>> it should "fail with a list of ignored files" as the documentation >>> states. >> >> It's still not clear, at least not to me. Should '-f' suppress the >> ignore setting of all involved submodules? That would make it a >> synonyme (or a superset) of --ignore-submodules=none. Or only if the >> submodule is explicitly named on command line? That seems fuzzy to me, >> and also more tricky to implement. > > Maybe my impression that doing "add" together with "commit" would be > easy wasn't correct after all. I won't object if you try to tackle > commit first (but I have the slight suspicion that similar questions > will arise concerning the "add"ish functionality in commit too. So > maybe after resolving those things might look clearer ;-) There is one big distinction. My patch for commit doesn't add any new problems. It just adds the --ignore-submodules argument, which is easy to implement and no unclear behavior decisions are needed. You are right that when specifying ignored submodules on commit's command line, there is the same problem as with git add. However, it's already there anyway. I don't feel in position to solve it, I'd just like to have "git commit --ignore-submodules=none". With git add however, changing it to honor settings from .gitmodules would change behavior people might be used to, so I would be afraid to do that. Btw add also has the problem already, but only if somebody configures the submodule's ignore setting in .git/config, rather than .gitmodules. I don't know how much real use case that is. As I see it, there are now these rather easy possibilities (sorted from the easiest): 1) Just teach commit the --ignore-submodules argument, as I proposed. 2) Teach both add and commit to --ignore-submodules, but dont add that problematic gitmodules_config() in add.c. 3) Teach both add and commit to --ignore-submodules, and also let add honor settings from .gitmodules, to make it more consistent with other commands. And, make add --force imply --ignore-submodules=none. I like both 1) and 2). I don't like 3), the problem of add with submodules' ignore setting is a bug IMHO (ignore=all in .git/config causes strange behavior, while ignore=all in .gitmodules is ignored), but not directly related to the --ignore-submodules param, and should be solved separately. -- 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's cooking in git.git (Mar 2014, #08; Mon, 31)
David Kastrup writes: > Junio C Hamano writes: > >> Junio C Hamano writes: >> >>> I haven't reverted the merge of that "submodule update" topic yet; I >>> should do that soonish. >>> ... >> >> Sigh... This is giving me a lot of headache. >> >> As 23d25e48 (submodule: explicit local branch creation in >> module_clone, 2014-01-26) has been in 'master' since fairly early >> during this cycle, a lot of topics that are not planned to be on the >> 'maint' branch has forked from the tip of 'master' and are now >> contaminated by that commit. >> >> I think I have a preparatory patch to correctly revert 00d4ff1a >> (Merge branch 'wt/doc-submodule-name-path-confusion-2', 2014-03-31) >> and 06c27689 (Merge branch 'wk/submodule-on-branch', 2014-02-27), >> and also a part of 384364b (Start preparing for Git 2.0, >> 2014-03-07), but I am not sure what to do with them ;-<)) > > Why not just revert on master? When merging with the topic branches, > the revert should then override the contamination. That was actually not the cumbersome part. I wanted to be very sure that the revert was correctly done, and one way I know to get an independent verification is to rebuild the master branch starting all the way back from the point before the problematic topic was merged to it. Some of the topics merged to 'master' after that point, however, were forked after that original problematic merge was made, so they needed to be rebuilt before I could do so. It is worth noting that this verification can also be done in a different way. You can start at 06c27689^1, and "cherry-pick -m1" (or "cherry-pick" for non-merge commits that update the release notes) the commits in "git rev-list --reverse --first-parent 06c27689..master" on top of it. That should result in the same tree object as a correct revert on top of 'master' would have. Because "cherry-pick -m1" loses the other parents, the resulting history does not reflect the reality, but I am not doing this in order to replace the history of the 'master' with the result but only to make sure that the final tree matches what would have happened if the topic were not merged to 'master', so it is sufficient for the purpose of this exercise. Hope it clarifies. -- 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 2/3] patch-id: document new behaviour
On Wed, Apr 02, 2014 at 11:18:26AM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > > On Mon, Mar 31, 2014 at 12:54:46PM -0700, Junio C Hamano wrote: > >> "Michael S. Tsirkin" writes: > >> > >> > The hash used is mostly an internal implementation detail, isn't it? > >> > >> Yes, but that does not mean we can break people who keep an external > >> database indexed with the patch-id by changing the default under > >> them, and "they can give --unstable option to work it around" is a > >> workaround, not a fix. Without this change, they did not have to do > >> anything. > >> > >> I would imagine that most of these people will be using the plain > >> vanilla "git show" output without any ordering or hunk splitting > >> when coming up with such a key. A possible way forward to allow the > >> configuration that corresponds to "-O" while not breaking > >> the existing users could be to make the "patch-id --stable" kick in > >> automatically (of course, do this only when the user did not give > >> the "--unstable" command line option to override) when we see the > >> orderfile configuration in the repository, or when we see that the > >> incoming patch looks like reordered (e.g. has multiple "diff --git" > >> header lines that refer to the same path, > > > > This would require us to track affected files in memory. > > Issue? > > Don't we already do that in order to handle a patch that touches the > same path more than once anyway? At least I don't see it in builtin/patch-id.c > I think a possibly larger issue > might be that you would still want to do the hashing in a single > pass so you may need to always keep two accumulated hashes, before > you can decide if the patch is or is not a straight-forward one and > use one of the two, but that hopefully should not require a rocket > scientist. But the issue is that equivalent patches would get a different hash. This is what I tried to fix, after all. So I think I prefer using an option and not a heuristic if you are fine with that. At some point in the future we might flip the default. -- MST -- 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.1] commit: add --ignore-submodules[=] parameter
Am 01.04.2014 23:59, schrieb Ronald Weiss: > On 1. 4. 2014 22:23, Jens Lehmann wrote: >> Am 01.04.2014 01:35, schrieb Ronald Weiss: >>> On 1. 4. 2014 0:50, Ronald Weiss wrote: On 31. 3. 2014 23:47, Ronald Weiss wrote: > On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann wrote: >> As Junio mentioned it would be great if you could teach the add >> command also honor the --ignore-submodule command line option in >> a companion patch. In the course of doing so you'll easily see if >> I was right or not, then please just order them in the most logical >> way. > > Well, if You (or Junio) really don't want my patch without another one > for git add, I may try to do it. However, git add does not even honor > the submodules' ignore setting from .gitmodules (just tested with git > 1.9.1: "git add -u" doesn't honor it, while "git commit -a" does). So > teaching git add the --ignore-submodules switch in current state > doesn't seem right to me. You might propose to add also support for > the ignore setting, to make "add -u" and "commit -a" more consistent. > That seems like a good idea, but the effort needed is getting bigger, Well, now I actually looked at it, and it was pretty easy after all. The changes below seem to enable support for both ignore setting in .gitmodules, and also --ignore-submodules switch, for git add, on top of my patch for commit. >>> >>> There is a catch. With the changes below, submodules are ignored by add >>> even if explitely named on command line (eg. "git add x" does nothing if x >>> is submodule with new commits, but with ignore=all in .gitmodules). >>> That doesn't seem right. >>> >>> Any ideas, what to do about that? When exactly should such submodule be >>> actually ignored? >> >> Me thinks git add should require the '-f' option to add an ignored >> submodule (just like it does for files) unless the user uses the >> '--ignore-submodules=none' option. And if neither of these are given >> it should "fail with a list of ignored files" as the documentation >> states. > > It's still not clear, at least not to me. Should '-f' suppress the > ignore setting of all involved submodules? That would make it a > synonyme (or a superset) of --ignore-submodules=none. Or only if the > submodule is explicitly named on command line? That seems fuzzy to me, > and also more tricky to implement. Maybe my impression that doing "add" together with "commit" would be easy wasn't correct after all. I won't object if you try to tackle commit first (but I have the slight suspicion that similar questions will arise concerning the "add"ish functionality in commit too. So maybe after resolving those things might look clearer ;-) -- 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 2/3] patch-id: document new behaviour
"Michael S. Tsirkin" writes: > On Mon, Mar 31, 2014 at 12:54:46PM -0700, Junio C Hamano wrote: >> "Michael S. Tsirkin" writes: >> >> > The hash used is mostly an internal implementation detail, isn't it? >> >> Yes, but that does not mean we can break people who keep an external >> database indexed with the patch-id by changing the default under >> them, and "they can give --unstable option to work it around" is a >> workaround, not a fix. Without this change, they did not have to do >> anything. >> >> I would imagine that most of these people will be using the plain >> vanilla "git show" output without any ordering or hunk splitting >> when coming up with such a key. A possible way forward to allow the >> configuration that corresponds to "-O" while not breaking >> the existing users could be to make the "patch-id --stable" kick in >> automatically (of course, do this only when the user did not give >> the "--unstable" command line option to override) when we see the >> orderfile configuration in the repository, or when we see that the >> incoming patch looks like reordered (e.g. has multiple "diff --git" >> header lines that refer to the same path, > > This would require us to track affected files in memory. > Issue? Don't we already do that in order to handle a patch that touches the same path more than once anyway? I think a possibly larger issue might be that you would still want to do the hashing in a single pass so you may need to always keep two accumulated hashes, before you can decide if the patch is or is not a straight-forward one and use one of the two, but that hopefully should not require a rocket scientist. -- 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] ls-files: do not trust stat info if lstat() fails
Nguyễn Thái Ngọc Duy writes:
> If 'err' is non-zero, lstat() has failed. Consider the entry modified
> without passing the (unreliable) stat info to ce_modified() in this
> case.
>
> Noticed-by: Eric Sunshine
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> On Fri, Mar 28, 2014 at 11:04 AM, Eric Sunshine
> wrote:
> > On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy
> wrote:
> >> + err = lstat(ce->name, &st);
> >> + if (show_deleted && err) {
> >> + show_ce_entry(tag_removed, ce);
> >> + shown = 1;
> >> + }
> >> + if (show_modified && ce_modified(ce, &st, 0)) {
> >
> > Is it possible for the lstat() to have failed for some reason when we
> > get here? If so, relying upon 'st' is unsafe, isn't it?
>
> The chance of random stat making ce_modified() return false is pretty
> low, but you're right. This code is a copy from the old show_files().
> I'll fix it in the git-ls series. Meanwhile a patch for maint to fix
> the original function.
>
> builtin/ls-files.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 47c3880..e6bd00e 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -260,7 +260,7 @@ static void show_files(struct dir_struct *dir)
> err = lstat(ce->name, &st);
> if (show_deleted && err)
> show_ce_entry(tag_removed, ce);
> - if (show_modified && ce_modified(ce, &st, 0))
> + if (show_modified && (err || ce_modified(ce, &st, 0)))
> show_ce_entry(tag_modified, ce);
> }
> }
I am guessing that, even though this was discovered during the
development of list-files, is a fix applicable outside the context
of that series.
I do think the patched result is an improvement than the status quo,
but at the same time, I find it insufficient in the context of the
whole codepath. What if errno were other than ENOENT and we were
told to show_deleted (with or without show_modified)? We would end
up saying the path was deleted and modified at the same time, when
we do not know either is or is not true at all, because of the
failure to lstat() the path.
Wouldn't it be saner to add tag_unknown and do something like this
instead, I wonder?
builtin/ls-files.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 47c3880..af2ce99 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -46,6 +46,7 @@ static const char *tag_killed = "";
static const char *tag_modified = "";
static const char *tag_skip_worktree = "";
static const char *tag_resolve_undo = "";
+static const char *tag_unknown = "";
static void write_name(const char *name)
{
@@ -249,7 +250,7 @@ static void show_files(struct dir_struct *dir)
for (i = 0; i < active_nr; i++) {
const struct cache_entry *ce = active_cache[i];
struct stat st;
- int err;
+
if ((dir->flags & DIR_SHOW_IGNORED) &&
!ce_excluded(dir, ce))
continue;
@@ -257,11 +258,15 @@ static void show_files(struct dir_struct *dir)
continue;
if (ce_skip_worktree(ce))
continue;
- err = lstat(ce->name, &st);
- if (show_deleted && err)
- show_ce_entry(tag_removed, ce);
- if (show_modified && ce_modified(ce, &st, 0))
+ errno = 0;
+ if (lstat(ce->name, &st)) {
+ if (errno != ENOENT && errno != ENOTDIR)
+ show_ce_entry(tag_unknown, ce);
+ else if (show_deleted)
+ show_ce_entry(tag_removed, ce);
+ } else if (show_modified && ce_modified(ce, &st, 0)) {
show_ce_entry(tag_modified, ce);
+ }
}
}
}
@@ -533,6 +538,7 @@ int cmd_ls_files(int argc, const char **argv, const char
*cmd_prefix)
tag_killed = "K ";
tag_skip_worktree = "S ";
tag_resolve_undo = "U ";
+ tag_unknown = "! ";
}
if (show_modified || show_others || show_deleted || (dir.flags &
DIR_SHOW_IGNORED) || show_killed)
require_work_tree = 1;
--
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 feature request: Option to force Git to abort a checkout if working directory is dirty (i.e. disregarding the check for conflicts)
"Jonas Bang" writes: >> >> ... The default behaviour would cover their >> >> use case so your proposal would not hurt them, but I wonder if there >> >> are things you could do to help them as well, perhaps by allowing >> >> this new configuration to express something like "local changes in >> >> these paths are excempt from this new check". >> > >> > Yes, those people would probably use the default 'false' behavior as >> > it is already. If they however would like to use e.g. the 'true' or >> > 'include-untracked' setting as a configuration variable, then they can >> > use the command line option 'false' if they wish to do a 'git >> > checkout' even with modified files in the working tree. >> >> So in short, you are saying that "The added code necessary to implement >> this feature will not help them in any way, it is just that we >> will make sure it does not hurt them". > > I didn't realize they needed help. If so, then you could have just stated that way, instead of saying they have an escape hatch ;-) It is perfectly fine to answer to "I wonder if there are things you could do?" with "No, I am not going to help them with this series; I only make sure I do not hurt them." and that is perfectly fine, as long as: - you do not directly hurt them with your series; and - you do not make it harder for those who are interested in helping them to build on top of your work in the future. > How and who to decide if this is a reasonable feature request to accept? As this project primarily works on "scratch your own itch" basis, somebody who (1) thinks that the proposed feature is worth having in our system and (2) is interested in working on it will pick it up. If nobody is interested, then usually nothing happens. -- 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: Q: ".git/HEAD" and ".git/refs/heads"
"Ulrich Windl" writes:
> Hi!
>
> I have a small question: After a "git gc" with last commit being "[shared
> 2679648]" I found this:
>> cat .git/HEAD
> ref: refs/heads/shared
>> cat .git/refs/heads/shared
> cat: .git/refs/heads/shared: No such file or directory
>
> Is this intentional?
Yes.
> commit in these files: .git/logs/HEAD .git/logs/refs/heads/shared
> .git/info/refs .git/packed-refs
Yes, they are where the refs are stored. If you have many refs in your
repository, having one file per ref is inefficient. It's more efficient
for Git to have one big read-only file. When one ref is modified, the
.git/refs/heads/$branch file is re-created, and the packed entry is
ignored.
> if [ -d .git ]; then
> GIT_HEAD="$(<.git/HEAD)"
> GIT_BRANCH="${GIT_HEAD##*/}"
> GIT_HEAD="Git:$GIT_BRANCH-$(cut -c1-7 .git/${GIT_HEAD##*: })"
> fi
Don't access the filesystem. Ask Git.
GIT_FULL_BRANCH=$(git rev-parse --symbolic-full-name HEAD)
GIT_BRANCH="${GIT_FULL_BRANCH##*/}"
GIT_HEAD="Git:$GIT_BRANCH-$(git rev-parse --short HEAD)"
(Perhaps there's a simpler way without $GIT_FULL_BRANCH)
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 26/27] struct ref_update: Add type field
Junio C Hamano writes:
> I wonder if ref-transaction-commit can shrink its parameter list by
> accepting a single pointer to one ref_update?
Disregard this one. I was fooled into thinking that the function is
called with parameters such as update->old_sha1, update_flags,
update->type when looking at the hunk starting at l.3437; the called
function there is not ref-transaction-commit.
Sorry, and thanks.
>> @@ -3437,7 +3436,7 @@ int ref_transaction_commit(struct ref_transaction
>> *transaction,
>> (update->have_old ?
>> update->old_sha1 : NULL),
>> update->flags,
>> - &types[i], onerr);
>> + &update->type, onerr);
>> if (!update->lock) {
>> ret = 1;
>> goto cleanup;
--
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] pack-objects: do not reuse packfiles without --delta-base-offset
Jeff King writes:
> When we are sending a packfile to a remote, we currently try
> to reuse a whole chunk of packfile without bothering to look
> at the individual objects. This can make things like initial
> clones much lighter on the server, as we can just dump the
> packfile bytes.
>
> However, it's possible that the other side cannot read our
> packfile verbatim. For example, we may have objects stored
> as OFS_DELTA, but the client is an antique version of git
> that only understands REF_DELTA. We negotiate this
> capability over the fetch protocol. A normal pack-objects
> run will convert OFS_DELTA into REF_DELTA on the fly, but
> the "reuse pack" code path never even looks at the objects.
The above makes it sound like "reuse pack" codepath is broken. Is it
too much hassle to peek at the initial bytes of each object to see
how they are encoded? Would it be possible to convert OFS_DELTA to
REF_DELTA on the fly on that codepath as well, instead of disabling
the reuse altogether?
> This patch disables packfile reuse if the other side is
> missing any capabilities that we might have used in the
> on-disk pack. Right now the only one is OFS_DELTA, but we
> may need to expand in the future (e.g., if packv4 introduces
> new object types).
>
> We could be more thorough and only disable reuse in this
> case when we actually have an OFS_DELTA to send, but:
>
> 1. We almost always will have one, since we prefer
> OFS_DELTA to REF_DELTA when possible. So this case
> would almost never come up.
>
> 2. Looking through the objects defeats the purpose of the
> optimization, which is to do as little work as possible
> to get the bytes to the remote.
>
> Signed-off-by: Jeff King
> ---
> I happened to be fooling around with git v1.4.0 today, and noticed a
> problem fetching from GitHub. Pre-OFS_DELTA git versions are ancient by
> today's standard, but it's quite easy to remain compatible here, so I
> don't see why not.
And in theory, alternate implementations might not
> understand OFS_DELTA, though in practice I would consider such an
> implementation to be pretty crappy.
>
> builtin/pack-objects.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 7950c43..1503632 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2439,12 +2439,23 @@ static void loosen_unused_packed_objects(struct
> rev_info *revs)
> }
> }
>
> +/*
> + * This tracks any options which a reader of the pack might
> + * not understand, and which would therefore prevent blind reuse
> + * of what we have on disk.
> + */
> +static int pack_options_allow_reuse(void)
> +{
> + return allow_ofs_delta;
> +}
> +
> static int get_object_list_from_bitmap(struct rev_info *revs)
> {
> if (prepare_bitmap_walk(revs) < 0)
> return -1;
>
> - if (!reuse_partial_packfile_from_bitmap(
> + if (pack_options_allow_reuse() &&
> + !reuse_partial_packfile_from_bitmap(
> &reuse_packfile,
> &reuse_packfile_objects,
> &reuse_packfile_offset)) {
--
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] rev-parse: fix typo in example on manpage
Am 02.04.2014 19:32, schrieb Junio C Hamano: René Scharfe writes: --- Documentation/git-rev-parse.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Thanks; I'll forge your Sign-off ;-) Oops, sorry, and thanks. Signed-off-by: Rene Scharfe diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt index e05e6b3..377d9d7 100644 --- a/Documentation/git-rev-parse.txt +++ b/Documentation/git-rev-parse.txt @@ -363,7 +363,7 @@ usage: some-command [options] ... -h, --helpshow the help --foo some nifty option --foo --bar ... some cool option --bar with an argument ---baranother cool option --baz with a named argument +--bazanother cool option --baz with a named argument --qux[=]qux may take a path argument but has meaning by itself An option group Header -- 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] rev-parse: fix typo in example on manpage
René Scharfe writes: > --- > Documentation/git-rev-parse.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks; I'll forge your Sign-off ;-) > > diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt > index e05e6b3..377d9d7 100644 > --- a/Documentation/git-rev-parse.txt > +++ b/Documentation/git-rev-parse.txt > @@ -363,7 +363,7 @@ usage: some-command [options] ... > -h, --helpshow the help > --foo some nifty option --foo > --bar ... some cool option --bar with an argument > ---baranother cool option --baz with a named argument > +--bazanother cool option --baz with a named argument > --qux[=]qux may take a path argument but has meaning by > itself > > An option group Header -- 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 13/22] config: change write_error() to take a (struct lock_file *) argument
Michael Haggerty writes:
> Reduce the amount of code that has to know about the lock_file's
> filename field.
>
> Signed-off-by: Michael Haggerty
> ---
> config.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/config.c b/config.c
> index 6821cef..1ea3f39 100644
> --- a/config.c
> +++ b/config.c
> @@ -1303,9 +1303,9 @@ static int store_aux(const char *key, const char
> *value, void *cb)
> return 0;
> }
>
> -static int write_error(const char *filename)
> +static int write_error(struct lock_file *lk)
> {
The earlier one would have been usable for reporting an error while
writing any file, but the caller must hold a lock file on it with a
new one. Would this change warrant a renaming of the function, I
wonder.
It is a file-scope static, so all callers know about how they are
supposed to call it, hence the keeping the original name would be
OK, I would think.
This hunk triggered my smello-meter, primarily because "write-error"
would not be the name I would pick for this function if I were
writing everything in this file from scratch (before or after this
particular patch).
> - error("failed to write new configuration file %s", filename);
> + error("failed to write new configuration file %s", lk->filename);
>
> /* Same error code as "failed to rename". */
> return 4;
> @@ -1706,7 +1706,7 @@ out_free:
> return ret;
>
> write_err_out:
> - ret = write_error(lock->filename);
> + ret = write_error(lock);
> goto out_free;
>
> }
> @@ -1821,7 +1821,7 @@ int git_config_rename_section_in_file(const char
> *config_filename,
> }
> store.baselen = strlen(new_name);
> if (!store_write_section(out_fd, new_name)) {
> - ret = write_error(lock->filename);
> + ret = write_error(lock);
> goto out;
> }
> /*
> @@ -1847,7 +1847,7 @@ int git_config_rename_section_in_file(const char
> *config_filename,
> continue;
> length = strlen(output);
> if (write_in_full(out_fd, output, length) != length) {
> - ret = write_error(lock->filename);
> + ret = write_error(lock);
> goto out;
> }
> }
--
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 11/22] lockfile: define a constant LOCK_SUFFIX_LEN
Michael Haggerty writes:
> Signed-off-by: Michael Haggerty
> ---
> lockfile.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lockfile.c b/lockfile.c
> index 4a9ceda..4e3ada8 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -178,14 +178,17 @@ static char *resolve_symlink(char *p, size_t s)
> return p;
> }
>
> +/* We append ".lock" to the filename to derive the lockfile name: */
> +#define LOCK_SUFFIX_LEN 5
>
> static int lock_file(struct lock_file *lk, const char *path, int flags)
> {
> /*
> - * subtract 5 from size to make sure there's room for adding
> - * ".lock" for the lock file name
> + * subtract LOCK_SUFFIX_LEN from size to make sure there's
> + * room for adding ".lock" for the lock file name:
>*/
> - static const size_t max_path_len = sizeof(lk->filename) - 5;
> + static const size_t max_path_len = sizeof(lk->filename) -
> +LOCK_SUFFIX_LEN;
SP{8,} in the indent; I've cleaned it up while queueing.
Thanks.
--
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 22/22] lockfile: allow new file contents to be written while retaining lock
Michael Haggerty writes:
> +static int open_staging_file(struct lock_file *lk)
> +{
> + strbuf_setlen(&lk->staging_filename, lk->filename.len);
> + strbuf_addstr(&lk->staging_filename, ".new");
> + lk->fd = open(lk->staging_filename.buf, O_RDWR | O_CREAT | O_EXCL,
> 0666);
> + if (lk->fd < 0) {
> + return -1;
> + }
All the other "if (lk->fd < 0)" calls reset_lock_file(lk). Is it an
intentional omission that this one does not?
If so, please drop the extraneous {} around the single "return -1"
statement.
I also share the same puzzlement in Peff's review.
--
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 18/22] lockfile: also keep track of the filename of the file being locked
Michael Haggerty writes: > This reduces the amount of string editing gyrations and makes it > unnecessary for callers to know how to derive the filename from the > lock_filename. Hmph. Is it worth duplicating the whole thing? If you are planning to break the invariant lock_filename === filename + LOCK_SUFFIX in future changes in the series, it is understandable, though. -- 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 14/22] lockfile: use strbufs when handling (most) paths
Michael Haggerty writes: > Change struct lock_file's filename field from a fixed-length buffer > into a strbuf. Good. As I allued to in a review on an unrelated patch, I do not think it is a good idea to name the lock filename field "lock_filename" in a structure that is about a lockfile, though. -- 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 07/22] lock_file(): always add lock_file object to lock_file_list
Jeff King writes: > So for a moment, lk->filename contains the name of the valuable file we > are locking. If we get a signal at that moment, do we accidentally > delete it in remove_lock_file? > > I think the answer is "no", because we check lk->owner before deleting, > which will not match our pid (it should generally be zero due to xcalloc > or static initialization, though perhaps we should clear it here). That "generally be zero" no longer holds since 2/22, no? > But that makes me wonder about the case of a reused lock. It will have > lk->owner set from a previous invocation, and would potentially suffer > from this problem. In other words, I think the change you are > introducing does not have the problem, but the existing code does. :-/ Yes, I tend to agree. > I didn't reproduce it experimentally, though. We should be able to just > > lk->owner = 0; > > before the initial strcpy to fix it, I would think. > > -Peff -- 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/22] rollback_lock_file(): set fd to -1
Jeff King writes: > On Tue, Apr 01, 2014 at 05:58:12PM +0200, Michael Haggerty wrote: > >> When rolling back the lockfile, call close_lock_file() so that the >> lock_file's fd field gets set back to -1. This could help prevent >> confusion in the face of hypothetical future programming errors. > > This also solves a race. We could be in the middle of rollback_lock_file > when we get a signal, and double-close. It's probably not a big deal, > though, since nobody could have opened a new descriptor in the interim > that got the same number (so the second close will just fail silently). > > Still, this seems like a definite improvement. This is probably related to my comments on 2/22, but is "fd" the only thing that has a non-zero safe value? Perhaps lock_file_init() that clears the structure fields to 0/NULL and fd to -1, and a convenience function lock_file_alloc() that does xmalloc() and then calls lock_file_init() may help us a bit when the lockfile structure is reused? -- 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/22] try_merge_strategy(): remove redundant lock_file allocation
Jeff King writes: > On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote: > >> By the time the "if" block is entered, the lock_file instance from the >> main function block is no longer in use, so re-use that one instead of >> allocating a second one. >> >> Note that the "lock" variable in the "if" block used to shadow the >> "lock" variable at function scope, so the only change needed is to >> remove the inner definition. > > I wonder if this would also be simpler if "lock" were simply declared as > a static variable, and we drop the allocation entirely. I suppose that > does create more cognitive load, though, in that it is only correct if > the function is not recursive. On the other hand, the current code makes > a reader unfamiliar with "struct lock" wonder if there is a free(lock) > missing. Another thing that makes a reader wonder if this is a valid rewrite is if it is safe to reuse a lock_file structure, especially because the original gives a piece of memory _cleared_ with xcalloc(). The second invocation of hold_locked_index() is now done on a dirty piece of memory, and the reader needs to drill down the callchain to see if that is safe (and if not, hold_locked_index() and probably the underlying lock_file() needs to memset() it to NULs). -- 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 18/27] update-ref --stdin: Harmonize error messages
Michael Haggerty writes: > Junio, I incorporated your feedback (which so far has only affected > commit messages). I also rebased the patch series to the current > master. I pushed the result to GitHub [1]. I'll refrain from spamming > the list with v3 yet. Thanks; let us know when you are ready ;-) I finished reading the remainder of the v2, and I think I sent out what I found worth commenting on (either positive or negative). I think the next thing to convert to the transaction API would be the "ok we know the set of updates from the pusher; let's update all of them" in receive-pack? In a sense that is of a lot more real-world impact than the update-ref plumbing. Side note: honestly speaking, I was dissapointed to see that the ref updates by the receive-pack process was not included in the series when I saw the cover letter that said this was a series about transactional updates to refs. Anyway... There are a few things that need to be thought through. Making the update in receive-pack all-or-none is a behaviour change, even though it may be a good one. We may want to allow the user a way to ask for the traditional "reject only the ones that cannot be updated". It probably goes like this: - On the wire, a new "ref-update-aon" capability is advertised from receive-pack to send-pack and can be requested in the opposite direction. - On the "git push" side, a new "--all-or-none" option, and optionally a new "push.allOrNone" configuration, is used to request the "ref-update-aon" capability over the wire. - On the receive-pack side, a new "receive.allOrNone" configuration can be used to always update refs in all-or-none fashion, no matter what the pusher says. - The receive-pack uses the ref transaction to update the refs in all-or-none fashion if it has receive.allOrNone, or both sides agree to use ref-update-aon in the capability exchange. If not, it updates the refs in some-may-succeed-some-may-fail fashion, one by one. Or something like that. -- 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
[no subject]
Good day, Please can I talk to you about interesting business deal? I will be happy if you permit. I wait to hear from you soon. Regards, Kalifah -- 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
Q: ".git/HEAD" and ".git/refs/heads"
Hi!
I have a small question: After a "git gc" with last commit being "[shared
2679648]" I found this:
> cat .git/HEAD
ref: refs/heads/shared
> cat .git/refs/heads/shared
cat: .git/refs/heads/shared: No such file or directory
Is this intentional? How does Git find the numeric commit for HEAD?
Using find, I found my commit in these files:
.git/logs/HEAD
.git/logs/refs/heads/shared
.git/info/refs
.git/packed-refs
Before "git gc" I found the commit ID in HEAD.
Why I'm worrying?: I tried to make a simple script that finds out the current
HEAD-commit like this:
if [ -d .git ]; then
GIT_HEAD="$(<.git/HEAD)"
GIT_BRANCH="${GIT_HEAD##*/}"
GIT_HEAD="Git:$GIT_BRANCH-$(cut -c1-7 .git/${GIT_HEAD##*: })"
fi
Then $GIT_HEAD was something like "Git:shared-863962c".
Should I use code like this:?
awk '$2 == "refs/heads/shared" { print $1 }' .git/info/refs
Of course I'd be most pleased if there was some git builtin to get that (I read
the manual without success).
Using an older version of Git (git-1.7.12)...
Regards,
Ulrich Windl
--
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 21/22] lockfile: extract a function reset_lock_file()
On 04/02/2014 09:06 AM, Eric Sunshine wrote:
> On Tue, Apr 1, 2014 at 11:58 AM, Michael Haggerty
> wrote:
>> Signed-off-by: Michael Haggerty
>> ---
>> lockfile.c | 31 ---
>> 1 file changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/lockfile.c b/lockfile.c
>> index 852d717..c06e134 100644
>> --- a/lockfile.c
>> +++ b/lockfile.c
>> @@ -85,6 +85,14 @@ static void remove_lock_file_on_signal(int signo)
>> raise(signo);
>> }
>>
>> +static void reset_lock_file(struct lock_file *lk)
>> +{
>> + lk->fd = -1;
>> + strbuf_setlen(&lk->filename, 0);
>> + strbuf_setlen(&lk->staging_filename, 0);
>
> strbuf_reset() perhaps?
Thanks, Eric. For some reason I always have it in the back of my head
that strbuf_reset() frees the memory associated with the strbuf, but of
course that is strbuf_release() that I'm thinking of.
I just fixed all strbuf_setlen(..., 0) -> strbuf_reset(...) throughout
the patch series. The fix will be in the re-roll.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.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] update-ref: fail create operation over stdin if ref already exists
On 04/02/2014 04:09 AM, Michael Haggerty wrote: > From: Aman Gupta [snip] > @@ -147,6 +147,7 @@ static void parse_cmd_create(const char *next) > struct ref_update *update; > > update = update_alloc(); > + update->have_old = 1; Looks good. > +test_expect_success 'stdin -z create ref fails when ref exists' ' Strictly speaking we should have a non-z mode test too. Thanks, -Brad -- 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 in git-diff output
"rocketscienc01100101 ." writes:
> http://i.imgur.com/BoJSjm9.png
>
> Here's a screenshot that shows the problem.
(better cut-and-paste the text than sending a PNG image)
> There's always a misplaced line in the output (most of the time
> a[href^=tel] { }), no matter where in the file the changes are.
The part after the @@ are ignored by patch tools. They are here just for
convenience. They are a guess of what the patch hunk belongs to. For
C/Java/Ada/... programs, it's the function name. Git does not know about
CSS syntax, so it guesses wrong (last line starting with a letter I
guess, not sure exactly what happens when Git doesn't know the syntax).
But don't worry, these are juste hints for human, they are harmless.
> Sometimes it's even in the wrong position, above the @@ numbers.
That is strange. Do you have a way to reproduce this?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 12/22] delete_ref_loose(): don't muck around in the lock_file's filename
On 04/01/2014 10:21 PM, Jeff King wrote:
> On Tue, Apr 01, 2014 at 05:58:20PM +0200, Michael Haggerty wrote:
>
>> It's bad manners. Especially since, if unlink_or_warn() failed, the
>> memory wasn't restored to its original contents.
>>
>> So make our own copy to work with.
>
> Sounds good...
>
>> if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
>> -/* loose */
>> -int err, i = strlen(lock->lk->filename) - 5; /* .lock */
>> -
>> -lock->lk->filename[i] = 0;
>> -err = unlink_or_warn(lock->lk->filename);
>> -lock->lk->filename[i] = '.';
>> +/*
>> + * loose. The loose file name is the same as the
>> + * lockfile name, minus ".lock":
>> + */
>> +char *loose_filename = xmemdupz(lock->lk->filename,
>> +strlen(lock->lk->filename) - 5);
>> +int err = unlink_or_warn(loose_filename);
>> +free(loose_filename);
>
> Should we be using LOCK_SUFFIX_LEN from the previous commit here?
LOCK_SUFFIX_LEN is not in scope to this file, and I think it should stay
that way. But never fear; this figuring-out-filename-from-lockfile-name
nonsense is gone by the end of the patch series.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.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 09/22] api-lockfile: expand the documentation
On 04/01/2014 10:19 PM, Jeff King wrote: > On Tue, Apr 01, 2014 at 05:58:17PM +0200, Michael Haggerty wrote: > >> +unable_to_lock_error:: >> + >> +Emit an error describing that there was an error locking the >> +specified path. The err parameter should be the errno of the >> +problem that caused the failure. >> + >> +unable_to_lock_index_die:: >> + >> +Like `unable_to_lock_error()`, but also `die()`. > > Should this last one lost the "index" in its name? I think it is > vestigial at this point. Yes. Will be done in re-roll. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 in git-diff output
http://i.imgur.com/BoJSjm9.png
Here's a screenshot that shows the problem. There's always a misplaced
line in the output (most of the time a[href^=tel] { }), no matter
where in the file the changes are.
Sometimes it's even in the wrong position, above the @@ numbers.
I'd naturally expect the a[href^=tel] part to not show up at all
unless I make changes there.
On Tue, Apr 1, 2014 at 12:49 PM, rocketscienc01100101 .
wrote:
> I tried to get a diff between HEAD and the current version of my
> project, so I did "git diff".
>
> It's a web project with a CSS file that contains the following CSS rule:
>
> a[href^=tel] {
> color:inherit;
> text-decoration:none;
> }
>
> Now, whenever I do "git diff", it will always show the a[href^=tel]
> part and mess up the output, even when I didn't change anything near
> that line. The problem is easily reproducable in a newly created
> repository.
>
> git --version
> git version 1.9.1
--
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 in git-diff output
On Tue, Apr 1, 2014 at 12:49 PM, rocketscienc01100101 .
wrote:
> I tried to get a diff between HEAD and the current version of my
> project, so I did "git diff".
>
> It's a web project with a CSS file that contains the following CSS rule:
>
> a[href^=tel] {
> color:inherit;
> text-decoration:none;
> }
>
> Now, whenever I do "git diff", it will always show the a[href^=tel]
> part and mess up the output, even when I didn't change anything near
> that line. The problem is easily reproducable in a newly created
> repository.
>
> git --version
> git version 1.9.1
--
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/22] try_merge_strategy(): remove redundant lock_file allocation
On 04/01/2014 09:56 PM, Jeff King wrote: > On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote: > >> By the time the "if" block is entered, the lock_file instance from the >> main function block is no longer in use, so re-use that one instead of >> allocating a second one. >> >> Note that the "lock" variable in the "if" block used to shadow the >> "lock" variable at function scope, so the only change needed is to >> remove the inner definition. > > I wonder if this would also be simpler if "lock" were simply declared as > a static variable, and we drop the allocation entirely. I suppose that > does create more cognitive load, though, in that it is only correct if > the function is not recursive. On the other hand, the current code makes > a reader unfamiliar with "struct lock" wonder if there is a free(lock) > missing. Yes, a single lock_file object should suffice. I will make this change in the next version of the patch series. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 01/22] t3204: test deleting references when lock files already exist
On 04/01/2014 09:53 PM, Jeff King wrote: > On Tue, Apr 01, 2014 at 05:58:09PM +0200, Michael Haggerty wrote: > >> When deleting a reference, it might be that another process already >> holds the lock on the loose reference file and/or the packed-refs >> file. In those cases, there is no alternative but for the delete to >> fail. Verify that in such cases the reference values are left >> unchanged. >> >> But in fact, when the packed-refs file lock cannot be acquired, the >> loose reference file is deleted anyway, potentially leaving the >> reference with a changed value (its packed value, which might even >> point at an object that has been garbage collected). Thus one of the >> new tests is marked test_expect_failure. > > Nice find. If I understand correctly, the problem is at the plumbing > level, and this could also be demonstrated with update-ref? > > I don't think it is a big deal, but I was thrown for a minute by the use > of "git branch" (as in, "is this something special with branches, or is > this about all refs?"). Good point. Will change. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 v2 26/27] struct ref_update: Add type field
On 04/01/2014 10:03 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> This is temporary space for ref_transaction_commit(). >> >> Signed-off-by: Michael Haggerty >> --- > > I was about to complain to "*Add* type" that does not say what it is > used for at all, with "Please do not add something for unknown purpose > only to utilise it in a later patch". > > But that was before I noticed that these are already used and > realized that the change is about "moving what is recorded in the > type array, which is used to receive the existing reftype discovered > by calling resolve_ref_unsafe() in ref_transaction_commit() and not > used anywhere else, to a field of individual ref_update structure". > > So it was somewhat of a "Huh?", but perhaps it is OK. I will expand the comment in v3. > I wonder if ref-transaction-commit can shrink its parameter list by > accepting a single pointer to one ref_update? I don't understand this last point. ref_transaction_commit() has the following signature: int ref_transaction_commit(struct ref_transaction *transaction, const char *msg, enum action_on_err onerr) What change are you proposing? By the way, longer-term, I wonder if msg and maybe action_on_err should be set for each ref_update, rather than for a whole transaction. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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: git-status -- trying to understand all possible states
Jonathan Nieder writes: > - 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. It already does: part of the output may be translated to non-english. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] update-ref: fail create operation over stdin if ref already exists
From: Aman Gupta Signed-off-by: Aman Gupta Signed-off-by: Michael Haggerty --- My colleague Aman ran across this bug and wrote the fix. I didn't notice this bug, but I just verified that it is also fixed by my mh/ref-transaction patch series (albeit without a test case). Because the bug could cause somebody to overwrite a reference unintentionally, I propose that we apply this unintrusive fix to maint. When mh/ref-transaction makes it to a release, the bug will continue to be fixed, but in a different way. builtin/update-ref.c | 1 + t/t1400-update-ref.sh | 11 +++ 2 files changed, 12 insertions(+) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 1292cfe..5c208bb 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -147,6 +147,7 @@ static void parse_cmd_create(const char *next) struct ref_update *update; update = update_alloc(); + update->have_old = 1; if ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0]) update_store_ref_name(update, ref.buf); diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 6ffd82f..e130c52 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -820,7 +820,18 @@ test_expect_success 'stdin -z update ref fails with bad old value' ' test_must_fail git rev-parse --verify -q $c ' +test_expect_success 'stdin -z create ref fails when ref exists' ' + git update-ref $c $m && + git rev-parse "$c" >expect && + printf $F "create $c" "$m~1" >stdin && + test_must_fail git update-ref -z --stdin err && + grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err && + git rev-parse "$c" >actual && + test_cmp expect actual +' + test_expect_success 'stdin -z create ref fails with bad new value' ' + git update-ref -d "$c" && printf $F "create $c" "does-not-exist" >stdin && test_must_fail git update-ref -z --stdin err && grep "fatal: invalid new value for ref $c: does-not-exist" err && -- 1.9.0 -- 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 22/22] lockfile: allow new file contents to be written while retaining lock
On Tue, Apr 1, 2014 at 11:58 AM, Michael Haggerty wrote:
> Add a new option flag, LOCK_SEPARATE_STAGING_FILE, that can be passed
> to hold_lock_file_for_update() or hold_lock_file_for_append() to use a
> staging file that is independent of the lock file.
>
> Add a new function activate_staging_file() that activates the contents
> that have been written to the staging file without releasing the lock.
>
> This functionality can be used to ensure that changes to two files are
> seen by other processes in one order even if correctness requires the
> locks to be released in another order.
>
> Signed-off-by: Michael Haggerty
> ---
> diff --git a/lockfile.c b/lockfile.c
> index c06e134..336b914 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -271,19 +325,54 @@ int hold_lock_file_for_append(struct lock_file *lk,
> const char *path, int flags)
> return fd;
> }
>
> -int close_lock_file(struct lock_file *lk)
> +static int close_staging_file(struct lock_file *lk)
> {
> int fd = lk->fd;
> +
> lk->fd = -1;
> return close(fd);
> }
>
> -int commit_lock_file(struct lock_file *lk)
> +int close_lock_file(struct lock_file *lk)
> {
> - if (lk->fd >= 0 && close_lock_file(lk))
> - return -1;
> - if (rename(lk->staging_filename.buf, lk->filename.buf))
> + assert(!(lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE));
> + return close_staging_file(lk);
> +}
> +
> +int activate_staging_file(struct lock_file *lk)
> +{
> + int err;
> +
> + assert(lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE);
> + assert(lk->fd >= 0);
> + assert(lk->staging_filename.len);
> +
> + if (close_staging_file(lk))
> return -1;
> +
> + err = rename(lk->staging_filename.buf, lk->filename.buf);
> + strbuf_setlen(&lk->staging_filename, 0);
strbuf_reset()?
> +
> + return err;
> +}
> +
> +int commit_lock_file(struct lock_file *lk)
> +{
> + if (lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE) {
> + if (lk->staging_filename.len) {
> + assert(lk->fd >= 0);
> + if (activate_staging_file(lk))
> + return -1;
> + }
> + strbuf_addbuf(&lk->staging_filename, &lk->filename);
> + strbuf_addstr(&lk->staging_filename, ".lock");
> + unlink_or_warn(lk->staging_filename.buf);
> + } else {
> + if (lk->fd >= 0 && close_lock_file(lk))
> + return -1;
> + if (rename(lk->staging_filename.buf, lk->filename.buf))
> + return -1;
> + }
> reset_lock_file(lk);
> return 0;
> }
> @@ -318,10 +407,21 @@ int commit_locked_index(struct lock_file *lk)
>
> void rollback_lock_file(struct lock_file *lk)
> {
> - if (lk->filename.len) {
> - if (lk->fd >= 0)
> - close_lock_file(lk);
> - unlink_or_warn(lk->staging_filename.buf);
> - reset_lock_file(lk);
> + if (!lk->filename.len)
> + return;
> +
> + if (lk->fd >= 0)
> + close_staging_file(lk);
> +
> + if (lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE) {
> + if (lk->staging_filename.len) {
> + unlink_or_warn(lk->staging_filename.buf);
> + strbuf_setlen(&lk->staging_filename, 0);
strbuf_reset()?
> + }
> + strbuf_addbuf(&lk->staging_filename, &lk->filename);
> + strbuf_addstr(&lk->staging_filename, ".lock");
> }
> +
> + unlink_or_warn(lk->staging_filename.buf);
> + reset_lock_file(lk);
> }
> --
> 1.9.0
--
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 21/22] lockfile: extract a function reset_lock_file()
On Tue, Apr 1, 2014 at 11:58 AM, Michael Haggerty wrote:
> Signed-off-by: Michael Haggerty
> ---
> lockfile.c | 31 ---
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/lockfile.c b/lockfile.c
> index 852d717..c06e134 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -85,6 +85,14 @@ static void remove_lock_file_on_signal(int signo)
> raise(signo);
> }
>
> +static void reset_lock_file(struct lock_file *lk)
> +{
> + lk->fd = -1;
> + strbuf_setlen(&lk->filename, 0);
> + strbuf_setlen(&lk->staging_filename, 0);
strbuf_reset() perhaps?
> + lk->flags = LOCK_FLAGS_ON_LIST;
> +}
> +
> /*
> * path = absolute or relative path name
> *
> @@ -185,8 +193,7 @@ static int lock_file(struct lock_file *lk, const char
> *path, int flags)
>
> lk->fd = open(lk->staging_filename.buf, O_RDWR | O_CREAT | O_EXCL,
> 0666);
> if (lk->fd < 0) {
> - strbuf_setlen(&lk->filename, 0);
> - strbuf_setlen(&lk->staging_filename, 0);
> + reset_lock_file(lk);
> return -1;
> }
> if (adjust_shared_perm(lk->staging_filename.buf)) {
> @@ -273,17 +280,12 @@ int close_lock_file(struct lock_file *lk)
>
> int commit_lock_file(struct lock_file *lk)
> {
> - int err = 0;
> -
> if (lk->fd >= 0 && close_lock_file(lk))
> return -1;
> - if (rename(lk->staging_filename.buf, lk->filename.buf)) {
> - err = -1;
> - } else {
> - strbuf_setlen(&lk->filename, 0);
> - strbuf_setlen(&lk->staging_filename, 0);
> - }
> - return err;
> + if (rename(lk->staging_filename.buf, lk->filename.buf))
> + return -1;
> + reset_lock_file(lk);
> + return 0;
> }
>
> int hold_locked_index(struct lock_file *lk, int die_on_error)
> @@ -306,8 +308,8 @@ int commit_locked_index(struct lock_file *lk)
> return -1;
> if (rename(lk->staging_filename.buf, alternate_index_output))
> return -1;
> - strbuf_setlen(&lk->filename, 0);
> - strbuf_setlen(&lk->staging_filename, 0);
> +
> + reset_lock_file(lk);
> return 0;
> } else {
> return commit_lock_file(lk);
> @@ -320,7 +322,6 @@ void rollback_lock_file(struct lock_file *lk)
> if (lk->fd >= 0)
> close_lock_file(lk);
> unlink_or_warn(lk->staging_filename.buf);
> - strbuf_setlen(&lk->filename, 0);
> - strbuf_setlen(&lk->staging_filename, 0);
> + reset_lock_file(lk);
> }
> }
> --
> 1.9.0
>
> --
> 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
--
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
