Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'
Hi, On Wed, Apr 2, 2014 at 2:39 AM, Jonathan Nieder jrnie...@gmail.com wrote: (culling cc list) Hi, Christian Couder wrote: [Subject: Documentation: add documentation for 'git interpret-trailers'] Signed-off-by: Christian Couder chrisc...@tuxfamily.org This should be squashed into the patch that introduces the interpret-trailers command, IMHO (or if it should be reviewed separately, it can be an earlier patch). That way, someone looking at when the command was introduced and wanting to understand what it was originally meant to do has the information close by. Well, the series is not very long, so this patch is quite close to the beginning anyway. Thanks for picking up the 'git commit --fixes' topic and your steady work improving the series. [...] --- /dev/null +++ b/Documentation/git-interpret-trailers.txt @@ -0,0 +1,123 @@ +git-interpret-trailers(1) += + +NAME + +git-interpret-trailers - help add stuctured information into commit messages + +SYNOPSIS + +[verse] +'git interpret-trailers' [--trim-empty] [(token[(=|:)value])...] + +DESCRIPTION +--- +Help add RFC 822-like headers, called 'trailers', at the end of the +otherwise free-form part of a commit message. + +This command is a filter. It reads the standard input for a commit +message and applies the `token` arguments, if any, to this +message. The resulting message is emited on the standard output. Do you have an example? Does it work like this? $ git interpret-trailers 'signoff=Jonathan Nieder jrnie...@gmail.com' EOF foo bar baz qux EOF foo bar baz qux Signed-off-by: Jonathan Nieder jrnie...@gmail.com $ Yeah, that's the idea. But you need to run something like: $ git config trailer.signoff.key Signed-off-by: to configure it properly first. By the way trying your example, I found that it is not currently adding an empty line before the s-o-b. I will have a look. A short EXAMPLES section could help. Yeah, it is planned, but not yet implemented, as written in patch 0/11: The following features are planned but not yet implemented: - add more tests related to commands - add examples in documentation - integration with git commit If I am understanding it correctly, would a name like 'git add-trailers' work? It could work but it can modify, not just add trailers. How do I read back the trailers later? Why do you want to read them back? Right now it should be used in hooks to modify commit messages. [...] +By default, a 'token=value' or 'token:value' argument will be added +only if Why support both '=' and ':'? Using just one would make it easier to grep through scripts to see who is adding signoffs. That was already discussed previously. The reason is that people are used to token=value for command line arguments, but trailers appears in the result as token: value, so it is better for the user if we support both. Thanks, Christian. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 08/11] trailer: add tests for git interpret-trailers
On Wed, Apr 2, 2014 at 12:54 AM, Junio C Hamano gits...@pobox.com wrote: +test_expect_success '--trim-empty without config' ' + cat expected -\EOF + ack: Peff + Acked-by: Johan + EOF + git interpret-trailers --trim-empty ack = Peff Reviewed-by Acked-by: Johan sob: actual + test_cmp expected actual +' Let's avoid these overlong and unreadable lines by doing something like this (just one hunk shown for illustration): ack: Peff Acked-by: Johan EOF - git interpret-trailers --trim-empty ack = Peff Reviewed-by Acked-by: Johan sob: actual + git interpret-trailers --trim-empty ack = Peff \ + Reviewed-by Acked-by: Johan sob: actual test_cmp expected actual ' Ok. I've queued the series on 'pu' with the 'chop-overlong-lines' and another minor fix squashed in; hopefully we can merge to 'next' soonish. Great! Thanks, Christian. -- 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] pack-objects: do not reuse packfiles without --delta-base-offset
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.
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 p...@peff.net
---
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)) {
--
1.9.1.656.ge8a0637
--
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 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted
[]
diff --git a/lockfile.c b/lockfile.c
index c1af65b..1928e5e 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -148,9 +148,11 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
lock_file_list = lk;
lk-on_list = 1;
}
- if (adjust_shared_perm(lk-filename))
- return error(cannot fix permission bits on %s,
-lk-filename);
+ if (adjust_shared_perm(lk-filename)) {
+ error(cannot fix permission bits on %s, lk-filename);
+ rollback_lock_file(lk);
+ return -1;
Would it make sense to change the order of rollback() and error()?
Make the rollback first (and as early as possible) and whine then?
--
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
[]
diff --git a/refs.c b/refs.c
index 28d5eca..11ad23e 100644
--- a/refs.c
+++ b/refs.c
@@ -2485,12 +2485,14 @@ static int repack_without_ref(const char *refname)
static int delete_ref_loose(struct ref_lock *lock, int flag)
{
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);
At other places (lockfile.c) we use this
+#define LOCK_SUFFIX_LEN 5
I think it makes sense to move this definition to an include file (lockfile.h
??)
and use it here.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/22] delete_ref_loose(): don't muck around in the lock_file's filename
On Wed, Apr 02, 2014 at 08:52:17AM +0200, Torsten Bögershausen wrote: + /* + * 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); At other places (lockfile.c) we use this +#define LOCK_SUFFIX_LEN 5 I think it makes sense to move this definition to an include file (lockfile.h ??) and use it here. I had the same comment, but if you read through to patch 18, this manual munging goes away entirely, and the end result is much cleaner. -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 13/22] config: change write_error() to take a (struct lock_file *) argument
On 04/01/2014 05:58 PM, Michael Haggerty wrote:
Reduce the amount of code that has to know about the lock_file's
filename field.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
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)
Does the write_error() really need to know about struct lock_file ?
(The name of the function does not indicate that it is doing something
with lk)
And if, would it make sense to rename it into
write_error_and_do_something() ?
{
- 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 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: Bug in git-diff output
rocketscienc01100101 . rocketscienc01100...@gmail.com 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] update-ref: fail create operation over stdin if ref already exists
On 04/02/2014 04:09 AM, Michael Haggerty wrote: From: Aman Gupta a...@tmm1.net [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: [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 mhag...@alum.mit.edu
wrote:
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
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
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
[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
Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages
Michael Haggerty mhag...@alum.mit.edu 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
Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation
Jeff King p...@peff.net 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 04/22] rollback_lock_file(): set fd to -1
Jeff King p...@peff.net 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 07/22] lock_file(): always add lock_file object to lock_file_list
Jeff King p...@peff.net 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 14/22] lockfile: use strbufs when handling (most) paths
Michael Haggerty mhag...@alum.mit.edu 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 18/22] lockfile: also keep track of the filename of the file being locked
Michael Haggerty mhag...@alum.mit.edu 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 22/22] lockfile: allow new file contents to be written while retaining lock
Michael Haggerty mhag...@alum.mit.edu 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 11/22] lockfile: define a constant LOCK_SUFFIX_LEN
Michael Haggerty mhag...@alum.mit.edu writes:
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
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 13/22] config: change write_error() to take a (struct lock_file *) argument
Michael Haggerty mhag...@alum.mit.edu writes:
Reduce the amount of code that has to know about the lock_file's
filename field.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
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] rev-parse: fix typo in example on manpage
René Scharfe l@web.de 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] args... -h, --helpshow the help --foo some nifty option --foo --bar ... some cool option --bar with an argument ---bar arg another cool option --baz with a named argument +--baz arg another cool option --baz with a named argument --qux[=path]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
Am 02.04.2014 19:32, schrieb Junio C Hamano: René Scharfe l@web.de 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 l@web.de 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] args... -h, --helpshow the help --foo some nifty option --foo --bar ... some cool option --bar with an argument ---bar arg another cool option --baz with a named argument +--baz arg another cool option --baz with a named argument --qux[=path]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] pack-objects: do not reuse packfiles without --delta-base-offset
Jeff King p...@peff.net 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 p...@peff.net
---
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 v2 26/27] struct ref_update: Add type field
Junio C Hamano gits...@pobox.com 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: Q: .git/HEAD and .git/refs/heads
Ulrich Windl ulrich.wi...@rz.uni-regensburg.de 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: 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 em...@jonasbang.dk 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: [PATCH] ls-files: do not trust stat info if lstat() fails
Nguyễn Thái Ngọc Duy pclo...@gmail.com 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 sunsh...@sunshineco.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
On Fri, Mar 28, 2014 at 11:04 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com
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: [PATCH v3 2/3] patch-id: document new behaviour
Michael S. Tsirkin m...@redhat.com writes: On Mon, Mar 31, 2014 at 12:54:46PM -0700, Junio C Hamano wrote: Michael S. Tsirkin m...@redhat.com 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 -Oorderfile 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 v2.1] commit: add --ignore-submodules[=when] 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 jens.lehm...@web.de 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 addish 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
On Wed, Apr 02, 2014 at 11:18:26AM -0700, Junio C Hamano wrote: Michael S. Tsirkin m...@redhat.com writes: On Mon, Mar 31, 2014 at 12:54:46PM -0700, Junio C Hamano wrote: Michael S. Tsirkin m...@redhat.com 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 -Oorderfile 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: What's cooking in git.git (Mar 2014, #08; Mon, 31)
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@pobox.com writes: Junio C Hamano gits...@pobox.com 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 v2.1] commit: add --ignore-submodules[=when] 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 jens.lehm...@web.de 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 addish 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)
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
[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 21; 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
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 initialinitial git add initial git commit -minitial echo foofoo git add foo git commit -mfoo echo barbar 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
Re: socket_perror() bug?
On Mon, Mar 31, 2014 at 5:50 PM, Junio C Hamano gits...@pobox.com wrote: Thiago Farina tfrans...@gmail.com 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
Re: emacs buffer names
Duy Nguyen pclo...@gmail.com 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.c2, 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
