[PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname
Signed-off-by: Sun He sunheeh...@gmail.com
---
builtin/pack-objects.c | 17 +++--
bulk-checkin.c | 8 +---
pack-write.c | 20
pack.h | 2 +-
4 files changed, 25 insertions(+), 22 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..72fb82b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -803,7 +803,7 @@ static void write_pack_file(void)
if (!pack_to_stdout) {
struct stat st;
- char tmpname[PATH_MAX];
+ struct strbuf tmpname = STRBUF_INIT;
/*
* Packs are runtime accessed in their mtime
@@ -823,26 +823,22 @@ static void write_pack_file(void)
utb.modtime = --last_mtime;
if (utime(pack_tmp_name, utb) 0)
warning(failed utime() on %s: %s,
- tmpname, strerror(errno));
+ pack_tmp_name, strerror(errno));
}
- /* Enough space for -sha-1.pack? */
- if (sizeof(tmpname) = strlen(base_name) + 50)
- die(pack base name '%s' too long, base_name);
- snprintf(tmpname, sizeof(tmpname), %s-, base_name);
+ strbuf_addf(tmpname, %s-, base_name);
if (write_bitmap_index) {
bitmap_writer_set_checksum(sha1);
bitmap_writer_build_type_index(written_list,
nr_written);
}
- finish_tmp_packfile(tmpname, pack_tmp_name,
+ finish_tmp_packfile(tmpname, pack_tmp_name,
written_list, nr_written,
pack_idx_opts, sha1);
if (write_bitmap_index) {
- char *end_of_name_prefix = strrchr(tmpname, 0);
- sprintf(end_of_name_prefix, %s.bitmap,
sha1_to_hex(sha1));
+ strbuf_addf(tmpname, %s.bitmap
,sha1_to_hex(sha1));
stop_progress(progress_state);
@@ -851,10 +847,11 @@ static void write_pack_file(void)
bitmap_writer_select_commits(indexed_commits,
indexed_commits_nr, -1);
bitmap_writer_build(to_pack);
bitmap_writer_finish(written_list, nr_written,
-tmpname,
write_bitmap_options);
+tmpname.buf,
write_bitmap_options);
write_bitmap_index = 0;
}
+ strbuf_release(tmpname);
free(pack_tmp_name);
puts(sha1_to_hex(sha1));
}
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..98e651c 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -4,6 +4,7 @@
#include bulk-checkin.h
#include csum-file.h
#include pack.h
+#include strbuf.h
static int pack_compression_level = Z_DEFAULT_COMPRESSION;
@@ -23,7 +24,7 @@ static struct bulk_checkin_state {
static void finish_bulk_checkin(struct bulk_checkin_state *state)
{
unsigned char sha1[20];
- char packname[PATH_MAX];
+ struct strbuf packname = STRBUF_INIT;
int i;
if (!state-f)
@@ -43,8 +44,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state
*state)
close(fd);
}
- sprintf(packname, %s/pack/pack-, get_object_directory());
- finish_tmp_packfile(packname, state-pack_tmp_name,
+ strbuf_addf(packname, %s/pack/pack-, get_object_directory());
+ finish_tmp_packfile(packname, state-pack_tmp_name,
state-written, state-nr_written,
state-pack_idx_opts, sha1);
for (i = 0; i state-nr_written; i++)
@@ -54,6 +55,7 @@ clear_exit:
free(state-written);
memset(state, 0, sizeof(*state));
+ strbuf_release(packname);
/* Make objects we just wrote available to ourselves */
reprepare_packed_git();
}
diff --git a/pack-write.c b/pack-write.c
index 9b8308b..2204aa9 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)
return sha1fd(fd, *pack_tmp_name);
}
-void finish_tmp_packfile(char *name_buffer,
+void finish_tmp_packfile(struct strbuf *name_buffer,
const char *pack_tmp_name,
struct pack_idx_entry **written_list,
uint32_t nr_written,
@@ -344,7 +344,7 @@
Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream
On Fri, Feb 28, 2014 at 04:28:38PM +0900, Brian Gesiak wrote: I would be in favor of using test_i18ngrep, but it seems like this test file overwhelmingly uses test_(i18n)cmp, even when inspecting stderr output. We generally prefer cmp checks to grep checks, because they are more rigorous. However, when testing human-readable output which may change, sometimes being too specific can simply make the tests brittle and annoying. Using a forgiving regex that matches keywords can be helpful. So there's definitely some room for judgement. I think what you posted as v2 looks OK. Making double-sure that all tests pass when run with sh -x seems like a larger endeavor. Of course, I'd be happy to submit several patches if there's support for such a change. But as Peff points out it will be a large diff. Yeah, I don't think it's worth the effort. If you feel like continuing on this series, converting the warning() into a die() would be a much more productive use of time (but if you don't, I do not see any reason not to take the patches you've posted). -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: How to mark a complete sub-directory assume-unchanged/skip-worktree?
From: Duy Nguyen pclo...@gmail.com On Fri, Feb 28, 2014 at 6:46 AM, Philip Oakley philipoak...@iee.org wrote: Is there a particular bit of code I'd be worth studying for the partial index example to see how well it might fit my ideas? My last attempt was http://git.661346.n2.nabble.com/PATCH-00-17-Narrow-clone-v3-was-subtree-clone-tt5499879.html If you're interested in the index part then see 15/17 and maybe 03/17 and 04/17. I can try to rebase and publish the series somewhere if you want to try it out. -- Duy -- Thanks for that pointer. I'll look it out and see how well it matched my ideas, and reflect on any differences to pick up learning points early! Philip -- 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] repack: add `repack.honorpackkeep` config var
On Thu, Feb 27, 2014 at 10:04:44AM -0800, Junio C Hamano wrote:
I wonder if it makes sense to link it with pack.writebitmaps more
tightly, without even exposing it as a seemingly orthogonal knob
that can be tweaked, though.
I think that is because I do not fully understand the , because ...
part of the below:
This patch introduces an option to disable the
`--honor-pack-keep` option. It is not triggered by default,
even when pack.writeBitmaps is turned on, because its use
depends on your overall packing strategy and use of .keep
files.
If you ask --write-bitmap-index (or have pack.writeBitmaps on), you
do want the bitmap-index to be written, and unless you tell
pack-objects to ignore the .keep marker, it cannot do so, no?
Does the , because ... part above mean you may have an overall
packing strategy to use .keep file to not ever repack some subset of
the objects, so we will not silently explode the kept objects into a
new pack?
Exactly. The two features (bitmaps and .keep) are not compatible with
each other, so you have to prioritize one. If you are using static .keep
files, you might want them to continue being respected at the expense of
using bitmaps for that repo. So I think you want a separate option from
--write-bitmap-index to allow the appropriate flexibility.
The default is another matter. I think most people using .bitmaps on a
server would probably want to set repack.packKeptObjects. They would
want to repack often to take advantage of the .bitmaps anyway, so they
probably don't care about .keep files (any they see are due to races
with incoming pushes).
So we could do something like falling back to turning the option on if
--write-bitmap-index is on _and_ the user didn't specify
--pack-kept-objects. The existing default is mostly there because it is
the conservative choice (.keep files continue to do their thing as
normal unless you say otherwise). But the fallback thing would be one
less knob that bitmap users would need to turn in the common case.
Here's the interdiff for doing the fallback:
---
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3a3d84f..a8ddc7f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2139,7 +2139,9 @@ repack.usedeltabaseoffset::
repack.packKeptObjects::
If set to true, makes `git repack` act as if
`--pack-kept-objects` was passed. See linkgit:git-repack[1] for
- details. Defaults to false.
+ details. Defaults to `false` normally, but `true` if a bitmap
+ index is being written (either via `--write-bitmap-index` or
+ `pack.writeBitmaps`).
rerere.autoupdate::
When set to true, `git-rerere` updates the index with the
diff --git a/builtin/repack.c b/builtin/repack.c
index 49947b2..6b0b62d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -9,7 +9,7 @@
#include argv-array.h
static int delta_base_offset = 1;
-static int pack_kept_objects;
+static int pack_kept_objects = -1;
static char *packdir, *packtmp;
static const char *const git_repack_usage[] = {
@@ -190,6 +190,9 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
argc = parse_options(argc, argv, prefix, builtin_repack_options,
git_repack_usage, 0);
+ if (pack_kept_objects 0)
+ pack_kept_objects = write_bitmap;
+
packdir = mkpathdup(%s/pack, get_object_directory());
packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, (int)getpid());
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index f8431a8..b1eed5c 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -21,7 +21,7 @@ test_expect_success 'objects in packs marked .keep are not
repacked' '
objsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 |
sed -e s/^\([0-9a-f]\{40\}\).*/\1/)
mv pack-* .git/objects/pack/
- git repack -A -d -l
+ git repack --no-pack-kept-objects -A -d -l
git prune-packed
for p in .git/objects/pack/*.idx; do
idx=$(basename $p)
@@ -35,9 +35,9 @@ test_expect_success 'objects in packs marked .keep are not
repacked' '
test -z $found_duplicate_object
'
-test_expect_success '--pack-kept-objects duplicates objects' '
+test_expect_success 'writing bitmaps duplicates .keep objects' '
# build on $objsha1, $packsha1, and .keep state from previous
- git repack -Adl --pack-kept-objects
+ git repack -Adl
test_when_finished found_duplicate_object=
for p in .git/objects/pack/*.idx; do
idx=$(basename $p)
--
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: An idea for git bisect and a GSoC enquiry
Mh. Haven't thought of that. I have no experience with TK, so I'm having trouble digging up where the good and bad labels in the GUI are generated. I guess that a solution might involve writing a temporary file in $GIT_DIR called something like BISECT_LABELS in which the chosen labels are listed and reused across all tools that require them. (Sorry for sending this email twice, I thought I had sent it to the list as well!) On Wed, Feb 26, 2014 at 8:58 PM, Junio C Hamano gits...@pobox.com wrote: Jacopo Notarstefano jacopo.notarstef...@gmail.com writes: Does this make sense? Did I overlook some details? How does this solve the labels shown in git bisect visualize? -- 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: [RFC 0/3] Make git more user-friendly during a merge conflict
Jonathan Nieder jrnie...@gmail.com writes: Hi, Andrew Wong wrote: The first two patches are just about rewording a message, and adding messages to tell users to use git merge --abort to abort a merge. Sounds like a good idea. I look forward to reading the patches. We could stop here and hope that the users would read the messages, but I think git could be a bit more user-friendly. The last patch might be a bit more controversial. It changes the default behavior of git reset to default to git reset --merge during a merge conflict. I imagine that's what the user would want most of the time, and not git reset --mixed. I don't think that's a good idea. I'm not sure what new users would expect; As a newbie, I would like to know how to abort the merge, so I like this message. in any case, making the command context-dependent just makes the learning process harder imho. I like commands that do the right thing. So no, this would not be confusing. And for experienced users, this would be a bad regression. Backward incompatibility is a real concern. It might be best if git reset (with _no_ option) be made to error out, so all users have to specify what they want. The transition process Junio proposed sounds good to me. -- -- 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: An idea for git bisect and a GSoC enquiry
On Thu, Feb 27, 2014 at 12:18 PM, Michael Haggerty mhag...@alum.mit.edu wrote: I don't understand the benefit of adding a new command mark rather than continuing to use good, bad, plus new commands unfixed and fixed. Does this solve any problems? As Matthieu Moy remarked in a previous email, the main reason is extensibility: I prefer having a single command to assign new descriptive labels instead of having to patch git-bisect.sh to create new labels like fixed, unfixed, fast, slow... What happens if the user mixes, say, good and fixed in a single bisect session? I don't think that's an issue. If the user uses the label fixed instead of bad she will have a hard time remembering to use it every time she needs it, and maybe the output of git bisect will look very confusing, but what can git do? This is a semantic user input error, not a syntax one. I think it would be more convenient if git bisect would autodetect whether the history went from good to bad or vice versa. The algorithm could be: 1. Wait until the user has marked one commit bad and one commit good. 2. If a good commit is an ancestor of a bad one, then git bisect should announce I will now look for the first bad commit. If reversed, then announce I will now look for the first good commit. If neither commit is an ancestor of the other, then explain the situation and ask the user to run git bisect find-first-bad or git bisect find-first-good or to mark another commit bad or good. 3. If the user marks another commit, go back to step 2, also doing a consistency check to make sure that all of the ancestry relationships go in a consistent direction. 4. After the direction is clear, the old bisect algorithm can be used (though taking account of the direction). Obviously a lot of the output would have to be adjusted, as would the way that a bisect is visualized. I can't think of any fundamental problems with a scheme like this, and I think it would be easier to use than the unfixed/fixed scheme. But that is only my opinion; other opinions are undoubtedly available :-) I like this idea! It also looks fun to implement. A minor difference is that I'd rather die with an error on point 2) if there's no ancestorship relation between the two commits; if the user is asking for such a thing then she has a fundamental misconception of the state of her repository. By the way, although git bisect fixed/unfixed would be a very useful improvement, and has gone unimplemented for a lamentably long time, my personal feeling is that it has too meat in it to constitute a GSoC project by itself. Oh! Then in fact, as Christian Couder said, this project shouldn't be marked as easy. (Sorry for sending this email twice! I thought I had sent it to the list as well.) On Thu, Feb 27, 2014 at 12:18 PM, Michael Haggerty mhag...@alum.mit.edu wrote: On 02/26/2014 09:28 AM, Jacopo Notarstefano wrote: my name is Jacopo, a student developer from Italy, and I'm interested in applying to this years' Google Summer of Code. I set my eyes on the project called git-bisect improvements, in particular the subtask about swapping the good and bad labels when looking for a bug-fixing release. Hello and welcome! I have a very simple proposal for that: add a new mark subcommand. Here is an example of how it should work: 1) A developer wants to find in which commit a past regression was fixed. She start bisecting as usual with git bisect start. 2) The current HEAD has the bugfix, so she marks it as fixed with git bisect mark fixed. 3) She knows that HEAD~100 had the regression, so she marks it as unfixed with git bisect mark unfixed. 4) Now that git knows what the two labels are, it starts bisecting as usual. For compatibility with already written scripts, git bisect good and git bisect bad will alias to git bisect mark good and git bisect mark bad respectively. Does this make sense? Did I overlook some details? I don't understand the benefit of adding a new command mark rather than continuing to use good, bad, plus new commands unfixed and fixed. Does this solve any problems? What happens if the user mixes, say, good and fixed in a single bisect session? I think it would be more convenient if git bisect would autodetect whether the history went from good to bad or vice versa. The algorithm could be: 1. Wait until the user has marked one commit bad and one commit good. 2. If a good commit is an ancestor of a bad one, then git bisect should announce I will now look for the first bad commit. If reversed, then announce I will now look for the first good commit. If neither commit is an ancestor of the other, then explain the situation and ask the user to run git bisect find-first-bad or git bisect find-first-good or to mark another commit bad or good. 3. If the user marks another commit, go back to step 2, also doing a consistency check to make sure that all of the ancestry relationships go in a
Re: [PATCH] archive: add archive.restrictRemote option
On Thu, Feb 27, 2014 at 10:37:30AM -0800, Junio C Hamano wrote: Signed-off-by: Jeff King p...@peff.net Thanks. Do GitHub people have general aversion against signing off (or sending out, for that matter) their own patches, unless they were already active here before they joined GitHub, by the way? Mostly it is that I clean up the patches and commit messages before sending them out. Michael sends out his own patches because they are already perfect by the time I see them. :) I can certainly get S-O-B from GitHubbers, but my impression of the DCO is that it does not matter; as the first link in the signoff chain, I am certifying that the patch meets the licensing requirements. Of course, I know that because of my relationship to the author and our employer, which is something that isn't encoded in the headers. A S-O-B from the author would perhaps make it more obvious what happened. I like the general idea and this escape hatch would be a good thing to have. A few comments: - Seeing the word combination restrict+remote before reading the explanation made me think hmph, only allow remote archive from certain hosts but not from others? We are restricting the objects and only on the remote usage, not restricting remotes, so somebody else may be able to come up with a less misleading name. - It might be better to call the escape hatch allow something that defaults to false. It is merely the issue of perception, but having a knob to be limiting that defaults to true gives a wrong impression that in an ideal world remote archive ought to be loose and we are artificially limiting it by default. After reading your first point, I came up with archive.allowRemoteUnreachable, which also satisfies the second. I do not have a strong opinion. +archive.restrictRemote:: + If true, archives can only be requested by refnames. If false, As this does not affect local use of git archive, requested by refnames may need to be clarified further. Perhaps remote archives can be requested only for published refnames or something. I was hoping to be vague. If we really want to get into specifics, we should probably document the current rules (refnames, and sub-trees of refnames). It might be a good thing to document that anyway, though. And by doing so, it would become obvious why one would want to set this option. I'll see what I can come up with. -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: An idea for git bisect and a GSoC enquiry
This email was sent privately by Michael to me as a result of my previous error. I'm quoting it in its entirety so that he doesn't have to submit it twice. On Thu, Feb 27, 2014 at 8:32 PM, Michael Haggerty mhag...@alum.mit.edu wrote: Please forgive my typos and brevity; this was typed on a phone. Michael On February 27, 2014 5:16:40 PM CET, Jacopo Notarstefano jacopo.notarstef...@gmail.com wrote: On Thu, Feb 27, 2014 at 12:18 PM, Michael Haggerty mhag...@alum.mit.edu wrote: What happens if the user mixes, say, good and fixed in a single bisect session? I don't think that's an issue. If the user uses the label fixed instead of bad she will have a hard time remembering to use it every time she needs it, and maybe the output of git bisect will look very confusing, but what can git do? This is a semantic user input error, not a syntax one. - git could emit an error message and refuse to continue - git could interpret the command one way or the other, with or without a warning By my count that gives at least five possibilities. The feature cannot be implemented without choosing one. I think it would be more convenient if git bisect would autodetect whether the history went from good to bad or vice versa. The algorithm could be: 1. Wait until the user has marked one commit bad and one commit good. 2. If a good commit is an ancestor of a bad one, then git bisect should announce I will now look for the first bad commit. If reversed, then announce I will now look for the first good commit. If neither commit is an ancestor of the other, then explain the situation and ask the user to run git bisect find-first-bad or git bisect find-first-good or to mark another commit bad or good. 3. If the user marks another commit, go back to step 2, also doing a consistency check to make sure that all of the ancestry relationships go in a consistent direction. 4. After the direction is clear, the old bisect algorithm can be used (though taking account of the direction). Obviously a lot of the output would have to be adjusted, as would the way that a bisect is visualized. I can't think of any fundamental problems with a scheme like this, and I think it would be easier to use than the unfixed/fixed scheme. But that is only my opinion; other opinions are undoubtedly available :-) I like this idea! It also looks fun to implement. A minor difference is that I'd rather die with an error on point 2) if there's no ancestorship relation between the two commits; if the user is asking for such a thing then she has a fundamental misconception of the state of her repository. That is not correct. If there is a bug on one branch but not another, it is legitimate to ask when the bug was introduced, and git bisect can indeed handle this case today (think about how this could work, and try it!) By the way, although git bisect fixed/unfixed would be a very useful improvement, and has gone unimplemented for a lamentably long time, my personal feeling is that it has too meat in it to constitute a GSoC project by itself. Oh! Then in fact, as Christian Couder said, this project shouldn't be marked as easy. Sorry for the typo; I meant to say too LITTLE meat. -- Michael Haggerty mhag...@alum.mit.edu -- 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] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE
Signed-off-by: Sun He sunheeh...@gmail.com --- parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index 7b8d3fa..59a52b0 100644 --- a/parse-options.c +++ b/parse-options.c @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts) case OPTION_NEGBIT: case OPTION_SET_INT: case OPTION_SET_PTR: - case OPTION_NUMBER: + case OPTION_CMDMODE: if ((opts-flags PARSE_OPT_OPTARG) || !(opts-flags PARSE_OPT_NOARG)) err |= optbug(opts, should not accept an argument); -- 1.9.0.138.g2de3478.dirty Hi, When I was reading code yesterday, I came across this protential bug. parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option. According to the information the program says (Should not accept an argument), I think you should take this patch into consideration. Thanks. He Sun -- 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: Branch Name Case Sensitivity
Karsten Blees karsten.bl...@gmail.com writes: If I understand the issue correctly, the problem is that packed-refs are always case-sensitive, even if core.ignorecase=true. Perhaps that could be changed? if core.ignorecase=true, packed-refs should be compared with case-insensitive string compares. -- -- 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: An idea for git bisect and a GSoC enquiry
- git could emit an error message and refuse to continue - git could interpret the command one way or the other, with or without a warning By my count that gives at least five possibilities. The feature cannot be implemented without choosing one. Let me explain what I meant with an example. 1) The user starts bisecting with bisect start. 2) The user marks HEAD as good with git bisect mark good. 3) The user then marks HEAD~10 as fixed with git bisect mark fixed. 4) Git will then continue bisecting as usual with the labels good and fixed instead of bad and good respectively. This is very confusing, but is a result of a user semantic error, so no warning is emitted. After all, this might have been what the user wanted. That is not correct. If there is a bug on one branch but not another, it is legitimate to ask when the bug was introduced, and git bisect can indeed handle this case today (think about how this could work, and try it!) Interesting. I did not know that. Yes, I see how that might pan out, and why my idea is worse. Sorry for the typo; I meant to say too LITTLE meat. Ok. Not a big issue for me: I might squash another project together in my proposal. I've already seen one that piqued my interest: Unifying git branch -l, git tag -l, and git for-each-ref. (Sorry for sending this email twice! I thought I had sent it to the list as well.) On Thu, Feb 27, 2014 at 8:32 PM, Michael Haggerty mhag...@alum.mit.edu wrote: Please forgive my typos and brevity; this was typed on a phone. Michael On February 27, 2014 5:16:40 PM CET, Jacopo Notarstefano jacopo.notarstef...@gmail.com wrote: On Thu, Feb 27, 2014 at 12:18 PM, Michael Haggerty mhag...@alum.mit.edu wrote: What happens if the user mixes, say, good and fixed in a single bisect session? I don't think that's an issue. If the user uses the label fixed instead of bad she will have a hard time remembering to use it every time she needs it, and maybe the output of git bisect will look very confusing, but what can git do? This is a semantic user input error, not a syntax one. - git could emit an error message and refuse to continue - git could interpret the command one way or the other, with or without a warning By my count that gives at least five possibilities. The feature cannot be implemented without choosing one. I think it would be more convenient if git bisect would autodetect whether the history went from good to bad or vice versa. The algorithm could be: 1. Wait until the user has marked one commit bad and one commit good. 2. If a good commit is an ancestor of a bad one, then git bisect should announce I will now look for the first bad commit. If reversed, then announce I will now look for the first good commit. If neither commit is an ancestor of the other, then explain the situation and ask the user to run git bisect find-first-bad or git bisect find-first-good or to mark another commit bad or good. 3. If the user marks another commit, go back to step 2, also doing a consistency check to make sure that all of the ancestry relationships go in a consistent direction. 4. After the direction is clear, the old bisect algorithm can be used (though taking account of the direction). Obviously a lot of the output would have to be adjusted, as would the way that a bisect is visualized. I can't think of any fundamental problems with a scheme like this, and I think it would be easier to use than the unfixed/fixed scheme. But that is only my opinion; other opinions are undoubtedly available :-) I like this idea! It also looks fun to implement. A minor difference is that I'd rather die with an error on point 2) if there's no ancestorship relation between the two commits; if the user is asking for such a thing then she has a fundamental misconception of the state of her repository. That is not correct. If there is a bug on one branch but not another, it is legitimate to ask when the bug was introduced, and git bisect can indeed handle this case today (think about how this could work, and try it!) By the way, although git bisect fixed/unfixed would be a very useful improvement, and has gone unimplemented for a lamentably long time, my personal feeling is that it has too meat in it to constitute a GSoC project by itself. Oh! Then in fact, as Christian Couder said, this project shouldn't be marked as easy. Sorry for the typo; I meant to say too LITTLE meat. -- Michael Haggerty mhag...@alum.mit.edu -- 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: Branch Name Case Sensitivity
On 02/28/2014 12:38 AM, Lee Hopkins wrote: [...] Based Michael Haggerty's response, it seems that always using loose refs would be a better workaround. No, I answered the question what would be the disadvantages of using only packed refs?. Now I will answer the question what would be the disadvantages of using only loose refs?: 1. Efficiency. Any time all of the references have to be read, loose refs are far slower than packed refs. 2. Disk space and inode usage: loose refs consume one inode and one disk sector (typically 4k) each, whereas packed refs consume only one inode in total, and many packed refs can fit into each disk sector. After all, there is a reason that we have both packed refs and loose refs. The basic idea is to use packed refs for the bulk of references, especially cold references like tags that only change infrequently, but to store hot references as loose refs so that they can be modified cheaply. 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] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf
On Fri, Feb 28, 2014 at 2:58 AM, Faiz Kothari faiz.of...@gmail.com wrote:
Signed-off-by: Faiz Kothari faiz.of...@gmail.com
Notes:
I finally got what's happening, and why the errors were caused.
packname is supposed to contain the complete path to the .pack file.
Packs are stored as /path/to/SHA1.pack which I overlooked earlier.
After inspecting what is happening in pack-write.c:finish_tmp_packfile()
which indirectly modifies packname by appending the SHA1 and .pack to
packname
This is happening in these code snippets:
char *end_of_name_prefix = strrchr(name_buffer, 0);
and later
sprintf(end_of_name_prefix, %s.pack, sha1_to_hex(sha1));
name_buffer is packname.buf
Using const for the first argument of pack-write.c:finish_tmp_packfile()
doesnot raise any compile time warning or error and not any runtime
errors,
since the packname.buf is on heap and has extra space to which more char
can be written.
If this was not the case,
for e.g. passing a constant string and modifying it.
This will result in a segmentation fault.
---
This notes section is important to the ongoing email discussion,
however, it should be placed below the --- line so that it does not
become part of the recorded commit message when the patch is applied
via git am.
bulk-checkin.c |8 +---
pack-write.c |2 +-
pack.h |2 +-
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..bbdf1ec 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,7 @@ static struct bulk_checkin_state {
static void finish_bulk_checkin(struct bulk_checkin_state *state)
{
unsigned char sha1[20];
- char packname[PATH_MAX];
+ struct strbuf packname = STRBUF_INIT;
int i;
if (!state-f)
@@ -42,9 +42,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state
*state)
state-offset);
close(fd);
}
+ strbuf_addf(packname, %s/pack/pack-, get_object_directory());
+ strbuf_grow(packname, 40 + 5);
There are several problems with this. First, magic numbers 40 and 5
convey no meaning to the reader. At the very least, they should be
named constants or a comment should explain them. More seriously,
though, this code is fragile since it has far too intimate knowledge
of the inner workings of finish_tmp_packfile(). If the implementation
of finish_tmp_packfile() changes in the future such that it writes
more than 45 additional characters to the incoming buffer, this will
break.
Rather than coupling finish_bulk_checkin() and finish_tmp_packfile()
so tightly, consider finish_tmp_packfile() a black box which just
does its job and then propose ways to make things work without
finish_bulk_checkin() having to know how that job is done.
- sprintf(packname, %s/pack/pack-, get_object_directory());
- finish_tmp_packfile(packname, state-pack_tmp_name,
+ finish_tmp_packfile(packname.buf, state-pack_tmp_name,
state-written, state-nr_written,
state-pack_idx_opts, sha1);
for (i = 0; i state-nr_written; i++)
diff --git a/pack-write.c b/pack-write.c
index 605d01b..ac38867 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)
return sha1fd(fd, *pack_tmp_name);
}
-void finish_tmp_packfile(char *name_buffer,
+void finish_tmp_packfile(const char *name_buffer,
This is misleading and fragile. By specifying 'const',
finish_tmp_packfile() promises not to modify the content of the
incoming name_buffer, yet it breaks this promise by modifying the
buffer through the non-const end_of_name_prefix variable (after
dropping the 'const' via strrchr()).
const char *pack_tmp_name,
struct pack_idx_entry **written_list,
uint32_t nr_written,
--
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: [RFC 0/3] Make git more user-friendly during a merge conflict
On Fri, Feb 28, 2014 at 03:01:53AM -0600, Stephen Leake wrote: Jonathan Nieder jrnie...@gmail.com writes: And for experienced users, this would be a bad regression. Backward incompatibility is a real concern. It might be best if git reset (with _no_ option) be made to error out, so all users have to specify what they want. This is just as much of a regression (if less dangerous) as changing the default behaviour of git reset to touch the working tree. 'git reset' is a very, very common action for me and simply means 'reset [my index] [to HEAD]'. I frequently find myself resetting so that I can stage something a bit different to what I had originally intended. -- 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 v2 00/11] Use ALLOC_GROW() instead of inline code
Thank you for your remarks. In this patch I tried to take them into account. Dmitry S. Dolzhenko (11): builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW() bundle.c: change add_to_ref_list() to use ALLOC_GROW() cache-tree.c: change find_subtree() to use ALLOC_GROW() commit.c: change register_commit_graft() to use ALLOC_GROW() diff.c: use ALLOC_GROW() instead of inline code diffcore-rename.c: use ALLOC_GROW() instead of inline code patch-ids.c: change add_commit() to use ALLOC_GROW() replace_object.c: change register_replace_object() to use ALLOC_GROW() reflog-walk.c: use ALLOC_GROW() instead of inline code dir.c: change create_simplify() to use ALLOC_GROW() attr.c: change handle_attr_line() to use ALLOC_GROW() attr.c | 7 +-- builtin/pack-objects.c | 7 +-- bundle.c | 6 +- cache-tree.c | 6 +- commit.c | 8 ++-- diff.c | 12 ++-- diffcore-rename.c | 12 ++-- dir.c | 5 + patch-ids.c| 5 + reflog-walk.c | 13 +++-- replace_object.c | 8 ++-- 11 files changed, 17 insertions(+), 72 deletions(-) -- 1.8.5.3 -- 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 v2 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()
Signed-off-by: Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru
---
builtin/pack-objects.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..56a6fc8 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1213,12 +1213,7 @@ static int check_pbase_path(unsigned hash)
if (0 = pos)
return 1;
pos = -pos - 1;
- if (done_pbase_paths_alloc = done_pbase_paths_num) {
- done_pbase_paths_alloc = alloc_nr(done_pbase_paths_alloc);
- done_pbase_paths = xrealloc(done_pbase_paths,
- done_pbase_paths_alloc *
- sizeof(unsigned));
- }
+ ALLOC_GROW(done_pbase_paths, done_pbase_paths_num + 1,
done_pbase_paths_alloc);
done_pbase_paths_num++;
if (pos done_pbase_paths_num)
memmove(done_pbase_paths + pos + 1,
--
1.8.5.3
--
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 v2 02/11] bundle.c: change add_to_ref_list() to use ALLOC_GROW()
Signed-off-by: Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru
---
bundle.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/bundle.c b/bundle.c
index e99065c..1388a3e 100644
--- a/bundle.c
+++ b/bundle.c
@@ -14,11 +14,7 @@ static const char bundle_signature[] = # v2 git bundle\n;
static void add_to_ref_list(const unsigned char *sha1, const char *name,
struct ref_list *list)
{
- if (list-nr + 1 = list-alloc) {
- list-alloc = alloc_nr(list-nr + 1);
- list-list = xrealloc(list-list,
- list-alloc * sizeof(list-list[0]));
- }
+ ALLOC_GROW(list-list, list-nr + 1, list-alloc);
memcpy(list-list[list-nr].sha1, sha1, 20);
list-list[list-nr].name = xstrdup(name);
list-nr++;
--
1.8.5.3
--
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 v2 03/11] cache-tree.c: change find_subtree() to use ALLOC_GROW()
Signed-off-by: Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru
---
cache-tree.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/cache-tree.c b/cache-tree.c
index 0bbec43..30149d1 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -75,11 +75,7 @@ static struct cache_tree_sub *find_subtree(struct cache_tree
*it,
return NULL;
pos = -pos-1;
- if (it-subtree_alloc = it-subtree_nr) {
- it-subtree_alloc = alloc_nr(it-subtree_alloc);
- it-down = xrealloc(it-down, it-subtree_alloc *
- sizeof(*it-down));
- }
+ ALLOC_GROW(it-down, it-subtree_nr + 1, it-subtree_alloc);
it-subtree_nr++;
down = xmalloc(sizeof(*down) + pathlen + 1);
--
1.8.5.3
--
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 v2 04/11] commit.c: change register_commit_graft() to use ALLOC_GROW()
Signed-off-by: Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru
---
commit.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/commit.c b/commit.c
index 6bf4fe0..e004314 100644
--- a/commit.c
+++ b/commit.c
@@ -147,12 +147,8 @@ int register_commit_graft(struct commit_graft *graft, int
ignore_dups)
return 1;
}
pos = -pos - 1;
- if (commit_graft_alloc = ++commit_graft_nr) {
- commit_graft_alloc = alloc_nr(commit_graft_alloc);
- commit_graft = xrealloc(commit_graft,
- sizeof(*commit_graft) *
- commit_graft_alloc);
- }
+ ALLOC_GROW(commit_graft, commit_graft_nr + 1, commit_graft_alloc);
+ commit_graft_nr++;
if (pos commit_graft_nr)
memmove(commit_graft + pos + 1,
commit_graft + pos,
--
1.8.5.3
--
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 v2 05/11] diff.c: use ALLOC_GROW() instead of inline code
Signed-off-by: Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru
---
diff.c | 12 ++--
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/diff.c b/diff.c
index e800666..aebdfda 100644
--- a/diff.c
+++ b/diff.c
@@ -1361,11 +1361,7 @@ static struct diffstat_file *diffstat_add(struct
diffstat_t *diffstat,
{
struct diffstat_file *x;
x = xcalloc(sizeof (*x), 1);
- if (diffstat-nr == diffstat-alloc) {
- diffstat-alloc = alloc_nr(diffstat-alloc);
- diffstat-files = xrealloc(diffstat-files,
- diffstat-alloc * sizeof(x));
- }
+ ALLOC_GROW(diffstat-files, diffstat-nr + 1, diffstat-alloc);
diffstat-files[diffstat-nr++] = x;
if (name_b) {
x-from_name = xstrdup(name_a);
@@ -3965,11 +3961,7 @@ struct diff_queue_struct diff_queued_diff;
void diff_q(struct diff_queue_struct *queue, struct diff_filepair *dp)
{
- if (queue-alloc = queue-nr) {
- queue-alloc = alloc_nr(queue-alloc);
- queue-queue = xrealloc(queue-queue,
- sizeof(dp) * queue-alloc);
- }
+ ALLOC_GROW(queue-queue, queue-nr + 1, queue-alloc);
queue-queue[queue-nr++] = dp;
}
--
1.8.5.3
--
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 v2 08/11] replace_object.c: change register_replace_object() to use ALLOC_GROW()
Signed-off-by: Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru
---
replace_object.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/replace_object.c b/replace_object.c
index cdcaf8c..843deef 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -36,12 +36,8 @@ static int register_replace_object(struct replace_object
*replace,
return 1;
}
pos = -pos - 1;
- if (replace_object_alloc = ++replace_object_nr) {
- replace_object_alloc = alloc_nr(replace_object_alloc);
- replace_object = xrealloc(replace_object,
- sizeof(*replace_object) *
- replace_object_alloc);
- }
+ ALLOC_GROW(replace_object, replace_object_nr + 1, replace_object_alloc);
+ replace_object_nr++;
if (pos replace_object_nr)
memmove(replace_object + pos + 1,
replace_object + pos,
--
1.8.5.3
--
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 v2 09/11] reflog-walk.c: use ALLOC_GROW() instead of inline code
Affected functions: read_one_reflog(), add_commit_info()
Signed-off-by: Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru
---
reflog-walk.c | 13 +++--
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/reflog-walk.c b/reflog-walk.c
index b2fbdb2..879d2ed 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -26,11 +26,7 @@ static int read_one_reflog(unsigned char *osha1, unsigned
char *nsha1,
struct complete_reflogs *array = cb_data;
struct reflog_info *item;
- if (array-nr = array-alloc) {
- array-alloc = alloc_nr(array-nr + 1);
- array-items = xrealloc(array-items, array-alloc *
- sizeof(struct reflog_info));
- }
+ ALLOC_GROW(array-items, array-nr + 1, array-alloc);
item = array-items + array-nr;
memcpy(item-osha1, osha1, 20);
memcpy(item-nsha1, nsha1, 20);
@@ -114,11 +110,8 @@ static void add_commit_info(struct commit *commit, void
*util,
struct commit_info_lifo *lifo)
{
struct commit_info *info;
- if (lifo-nr = lifo-alloc) {
- lifo-alloc = alloc_nr(lifo-nr + 1);
- lifo-items = xrealloc(lifo-items,
- lifo-alloc * sizeof(struct commit_info));
- }
+
+ ALLOC_GROW(lifo-items, lifo-nr + 1, lifo-alloc);
info = lifo-items + lifo-nr;
info-commit = commit;
info-util = util;
--
1.8.5.3
--
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 v2 10/11] dir.c: change create_simplify() to use ALLOC_GROW()
Signed-off-by: Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru
---
dir.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/dir.c b/dir.c
index 98bb50f..4ae38e4 100644
--- a/dir.c
+++ b/dir.c
@@ -1341,10 +1341,7 @@ static struct path_simplify *create_simplify(const char
**pathspec)
for (nr = 0 ; ; nr++) {
const char *match;
- if (nr = alloc) {
- alloc = alloc_nr(alloc);
- simplify = xrealloc(simplify, alloc *
sizeof(*simplify));
- }
+ ALLOC_GROW(simplify, nr + 1, alloc);
match = *pathspec++;
if (!match)
break;
--
1.8.5.3
--
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 v2 11/11] attr.c: change handle_attr_line() to use ALLOC_GROW()
Signed-off-by: Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru
---
attr.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/attr.c b/attr.c
index 8d13d70..734222d 100644
--- a/attr.c
+++ b/attr.c
@@ -338,12 +338,7 @@ static void handle_attr_line(struct attr_stack *res,
a = parse_attr_line(line, src, lineno, macro_ok);
if (!a)
return;
- if (res-alloc = res-num_matches) {
- res-alloc = alloc_nr(res-num_matches);
- res-attrs = xrealloc(res-attrs,
- sizeof(struct match_attr *) *
- res-alloc);
- }
+ ALLOC_GROW(res-attrs, res-num_matches + 1, res-alloc);
res-attrs[res-num_matches++] = a;
}
--
1.8.5.3
--
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 v2 0/2] lifting upload-archive restrictions
On Fri, Feb 28, 2014 at 04:07:09AM -0500, Jeff King wrote: As this does not affect local use of git archive, requested by refnames may need to be clarified further. Perhaps remote archives can be requested only for published refnames or something. I was hoping to be vague. If we really want to get into specifics, we should probably document the current rules (refnames, and sub-trees of refnames). It might be a good thing to document that anyway, though. And by doing so, it would become obvious why one would want to set this option. I'll see what I can come up with. Here's a series to handle this; I think the end result is much nicer. I ended up calling the option uploadarchive.allowUnreachable. By moving it there instead of under archive, it is more clear that this is about the _serving_ end of the remote connection, and the word remote becomes redundant. [1/2]: docs: clarify remote restrictions for git-upload-archive [2/2]: add uploadarchive.allowUnreachable option -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
[PATCH v2 1/2] docs: clarify remote restrictions for git-upload-archive
Commits ee27ca4 and 0f544ee introduced rules by which git-upload-archive would restrict clients from accessing unreachable objects. However, we never documented those rules anywhere, nor their reason for being. Let's do so now. Signed-off-by: Jeff King p...@peff.net --- Documentation/git-archive.txt| 5 - Documentation/git-upload-archive.txt | 26 ++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt index b97aaab..cfa1e4e 100644 --- a/Documentation/git-archive.txt +++ b/Documentation/git-archive.txt @@ -65,7 +65,10 @@ OPTIONS --remote=repo:: Instead of making a tar archive from the local repository, - retrieve a tar archive from a remote repository. + retrieve a tar archive from a remote repository. Note that the + remote repository may place restrictions on which sha1 + expressions may be allowed in `tree-ish`. See + linkgit:git-upload-archive[1] for details. --exec=git-upload-archive:: Used with --remote to specify the path to the diff --git a/Documentation/git-upload-archive.txt b/Documentation/git-upload-archive.txt index d09bbb5..8ae65d8 100644 --- a/Documentation/git-upload-archive.txt +++ b/Documentation/git-upload-archive.txt @@ -20,6 +20,32 @@ This command is usually not invoked directly by the end user. The UI for the protocol is on the 'git archive' side, and the program pair is meant to be used to get an archive from a remote repository. +SECURITY + + +In order to protect the privacy of objects that have been removed from +history but may not yet have been pruned, `git-upload-archive` avoids +serving archives for commits and trees that are not reachable from the +repository's refs. However, because calculating object reachability is +computationally expensive, `git-upload-archive` implements a stricter +but easier-to-check set of rules: + + 1. Clients may request a commit or tree that is pointed to directly by + a ref. E.g., `git archive --remote=origin v1.0`. + + 2. Clients may request a sub-tree within a commit or tree using the + `ref:path` syntax. E.g., `git archive --remote=origin v1.0:Documentation`. + + 3. Clients may _not_ use other sha1 expressions, even if the end + result is reachable. E.g., neither a relative commit like `master^` + nor a literal sha1 like `abcd1234` is allowed, even if the result + is reachable from the refs. + +Note that rule 3 disallows many cases that do not have any privacy +implications. These rules are subject to change in future versions of +git, and the server accessed by `git archive --remote` may or may not +follow these exact rules. + OPTIONS --- directory:: -- 1.8.5.2.500.g8060133 -- 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 v2 2/2] add uploadarchive.allowUnreachable option
From: Scott J. Goldman scot...@github.com
In commit ee27ca4, we started restricting remote git-archive
invocations to only accessing reachable commits. This
matches what upload-pack allows, but does restrict some
useful cases (e.g., HEAD:foo). We loosened this in 0f544ee,
which allows `foo:bar` as long as `foo` is a ref tip.
However, that still doesn't allow many useful things, like:
1. Commits accessible from a ref, like `foo^:bar`, which
are reachable
2. Arbitrary sha1s, even if they are reachable.
We can do a full object-reachability check for these cases,
but it can be quite expensive if the client has sent us the
sha1 of a tree; we have to visit every sub-tree of every
commit in the worst case.
Let's instead give site admins an escape hatch, in case they
prefer the more liberal behavior. For many sites, the full
object database is public anyway (e.g., if you allow dumb
walker access), or the site admin may simply decide the
security/convenience tradeoff is not worth it.
This patch adds a new config option to disable the
restrictions added in ee27ca4. It defaults to off, meaning
there is no change in behavior by default.
Signed-off-by: Jeff King p...@peff.net
---
Documentation/config.txt | 7 +++
Documentation/git-upload-archive.txt | 6 ++
archive.c| 13 +++--
t/t5000-tar-tree.sh | 9 +
4 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 040197b..62f0a4e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2334,6 +2334,13 @@ transfer.unpackLimit::
not set, the value of this variable is used instead.
The default value is 100.
+uploadarchive.allowUnreachable::
+ If true, allow clients to use `git archive --remote` to request
+ any tree, whether reachable from the ref tips or not. See the
+ discussion in the `SECURITY` section of
+ linkgit:git-upload-archive[1] for more details. Defaults to
+ `false`.
+
uploadpack.hiderefs::
String(s) `upload-pack` uses to decide which refs to omit
from its initial advertisement. Use more than one
diff --git a/Documentation/git-upload-archive.txt
b/Documentation/git-upload-archive.txt
index 8ae65d8..cbef61b 100644
--- a/Documentation/git-upload-archive.txt
+++ b/Documentation/git-upload-archive.txt
@@ -46,6 +46,12 @@ implications. These rules are subject to change in future
versions of
git, and the server accessed by `git archive --remote` may or may not
follow these exact rules.
+If the config option `uploadArchive.allowUnreachable` is true, these
+rules are ignored, and clients may use arbitrary sha1 expressions.
+This is useful if you do not care about the privacy of unreachable
+objects, or if your object database is already publicly available for
+access via non-smart-http.
+
OPTIONS
---
directory::
diff --git a/archive.c b/archive.c
index 346f3b2..7d0976f 100644
--- a/archive.c
+++ b/archive.c
@@ -17,6 +17,7 @@ static char const * const archive_usage[] = {
static const struct archiver **archivers;
static int nr_archivers;
static int alloc_archivers;
+static int remote_allow_unreachable;
void register_archiver(struct archiver *ar)
{
@@ -257,7 +258,7 @@ static void parse_treeish_arg(const char **argv,
unsigned char sha1[20];
/* Remotes are only allowed to fetch actual refs */
- if (remote) {
+ if (remote !remote_allow_unreachable) {
char *ref = NULL;
const char *colon = strchr(name, ':');
int refnamelen = colon ? colon - name : strlen(name);
@@ -401,6 +402,14 @@ static int parse_archive_args(int argc, const char **argv,
return argc;
}
+static int git_default_archive_config(const char *var, const char *value,
+ void *cb)
+{
+ if (!strcmp(var, uploadarchive.allowunreachable))
+ remote_allow_unreachable = git_config_bool(var, value);
+ return git_default_config(var, value, cb);
+}
+
int write_archive(int argc, const char **argv, const char *prefix,
int setup_prefix, const char *name_hint, int remote)
{
@@ -411,7 +420,7 @@ int write_archive(int argc, const char **argv, const char
*prefix,
if (setup_prefix prefix == NULL)
prefix = setup_git_directory_gently(nongit);
- git_config(git_default_config, NULL);
+ git_config(git_default_archive_config, NULL);
init_tar_archiver();
init_zip_archiver();
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 05f011d..1cf0a4e 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -213,6 +213,15 @@ test_expect_success 'clients cannot access unreachable
commits' '
test_must_fail git archive --remote=. $sha1 remote.tar
'
+test_expect_success 'upload-archive can allow unreachable commits' '
+ test_commit
Re: [RFC 0/3] Make git more user-friendly during a merge conflict
Stephen Leake stephen_le...@stephe-leake.org writes: I like commands that do the right thing. So no, this would not be confusing. I _hate_ commands that think they know better than to do what they are told. In particular when doing destructive things. And just because _you_ like them does not mean they are not confusing. In the long run, it is much more confusing if you come to rely on some commands doing the right thing while in other cases, the actually written thing is done. do the right thing commands also tend to do the wrong thing occasionally with potentially disastrous results when they are used in scripts where the followup actions rely on the actual result. -- David Kastrup -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream
If you feel like continuing on this series, converting the warning() into a die() would be a much more productive use of time (but if you don't, I do not see any reason not to take the patches you've posted). I'd be happy to keep working on this. In fact, I think I have a patch for this ready. But just to clarify: I notice that the warning comes from install_branch_config, which gets used both for branch -u, but also in the side effect case I mentioned above. Is it possible to trigger this as part of such a case? I think maybe git branch -f --track foo foo would do it. If so, we should perhaps include a test that it does not break if we upgrade the -u case to an error. Do you mean that install_branch_config should continue to emit a warning in the side effect case? I'm not sure I agree--how is git branch -f --track foo foo less erroneous than git branch -u foo refs/heads/foo? Perhaps I'm missing some insight on how --track is used. The tests appear to already cover all instances in which install_branch_config is called, and bumping the warning to an error does not cause any test failures. - Brian Gesiak -- 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] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE
On Fri, Feb 28, 2014 at 6:15 PM, Michael Haggerty mhag...@alum.mit.edu wrote: On 02/28/2014 10:07 AM, Sun He wrote: Signed-off-by: Sun He sunheeh...@gmail.com --- parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index 7b8d3fa..59a52b0 100644 --- a/parse-options.c +++ b/parse-options.c @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts) case OPTION_NEGBIT: case OPTION_SET_INT: case OPTION_SET_PTR: - case OPTION_NUMBER: + case OPTION_CMDMODE: if ((opts-flags PARSE_OPT_OPTARG) || !(opts-flags PARSE_OPT_NOARG)) err |= optbug(opts, should not accept an argument); -- 1.9.0.138.g2de3478.dirty Hi, When I was reading code yesterday, I came across this protential bug. parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option. According to the information the program says (Should not accept an argument), I think you should take this patch into consideration. Thanks. He Sun Please resubmit this change in the proper format (as described by Eric Sunshine WRT another one of your patches). Also please remember to distinguish between information that should go in the commit log message, which will be stored permanently to the repository and help future developers understand your change, vs. side notes meant only for the mailing list. For example, for the log message, stuff like: OPTION_CMDMODE should be used when ... So change the mode to OPTION_CMDMODE so that ... vs. for the mailing list discussion: When I was reading code yesterday ... The former goes above the --- line and the latter goes directly below it. BTW, none of my comments should be taken to indicate whether the commit itself makes sense or not. I haven't checked that far. 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 . -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream
On Fri, Feb 28, 2014 at 07:44:10PM +0900, Brian Gesiak wrote: I notice that the warning comes from install_branch_config, which gets used both for branch -u, but also in the side effect case I mentioned above. Is it possible to trigger this as part of such a case? I think maybe git branch -f --track foo foo would do it. If so, we should perhaps include a test that it does not break if we upgrade the -u case to an error. Do you mean that install_branch_config should continue to emit a warning in the side effect case? I'm not sure I agree--how is git branch -f --track foo foo less erroneous than git branch -u foo refs/heads/foo? Perhaps I'm missing some insight on how --track is used. I'd be more worried about triggering it via the config. E.g.: git config branch.autosetupmerge always git branch -f foo foo Should the second command die? I admit I'm having a hard time coming up with a feasible reason why anyone would do branch -f foo foo in the first place. I just don't want to regress somebody else's workflow due to my lack of imagination. -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
[PATCH] branch.c: delete size check of newly tracked branch names
Since commit 6f084a56 the length of a newly tracked branch name was limited to 1019 = 1024 - 7 - 7 - 1 characters, a bound derived by having to store this name in a char[1024] called key with two strings of length at most 7 and a '\0' character. This was no longer necessary as of commit a9f2c136, which uses a strbuf (documented in Documentation/technical/api-strbuf.txt) to store this value. This patch removes this unneeded check and thus allows for branch names longer than 1019 characters. Signed-off-by: Jacopo Notarstefano jacopo.notarstef...@gmail.com --- Submitted as GSoC microproject #3. branch.c | 4 1 file changed, 4 deletions(-) diff --git a/branch.c b/branch.c index 723a36b..05feaff 100644 --- a/branch.c +++ b/branch.c @@ -114,10 +114,6 @@ static int setup_tracking(const char *new_ref, const char *orig_ref, struct tracking tracking; int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; - if (strlen(new_ref) 1024 - 7 - 7 - 1) - return error(_(Tracking not set up: name too long: %s), - new_ref); - memset(tracking, 0, sizeof(tracking)); tracking.spec.dst = (char *)orig_ref; if (for_each_remote(find_tracked_branch, tracking)) -- 1.9.0.1.g5abca64 -- 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] branch.c: delete size check of newly tracked branch names
This patch removes this unneeded check and thus allows for branch names longer than 1019 characters. Ach! I amended the commit in my local history to read Remove this unneded check and thus allow for branch names longer than 1019 characters, but for some reason git format-patch -1 --signoff isn't reflecting this change. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream
I just don't want to regress somebody else's workflow due to my lack of imagination. This makes a lot of sense to me, although as-is the function emits a warning and returns immediately (although with a successful status code), so I'm also stumped as to what kind of workflow this would be included in. In any case, if the jury's out on this one, I suppose the two patches I submitted are good to merge? Part of me thinks the bump from warning to error belongs in its own patch anyway. - Brian Gesiak -- 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 17/19] Portable alloca for Git
Hi! On Mon, 24 Feb 2014 20:21:49 +0400, Kirill Smelkov k...@mns.spb.ru wrote: Both autoconf and config.mak.uname configurations were updated. For autoconf, we are not bothering considering cases, when no alloca.h is available, but alloca() works some other way - its simply alloca.h is available and works or not, everything else is deep legacy. Sounds good for GNU Hurd, or any system using glibc (but have not explicitly tested your patch). For config.mak.uname, I've tried to make my almost-sure guess for where alloca() is available, but since I only have access to Linux it is the only change I can be sure about myself, with relevant to other changed systems people Cc'ed. diff --git a/config.mak.uname b/config.mak.uname index 7d31fad..71602ee 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -239,6 +243,7 @@ ifeq ($(uname_S),AIX) endif ifeq ($(uname_S),GNU) # GNU/Hurd + HAVE_ALLOCA_H = YesPlease NO_STRLCPY = YesPlease NO_MKSTEMPS = YesPlease HAVE_PATHS_H = YesPlease Acked-by: Thomas Schwinge tho...@codesourcery.com (GNU Hurd changes) Grüße, Thomas pgphu749DPUFO.pgp Description: PGP signature
Re: [PATCH] branch.c: delete size check of newly tracked branch names
Nice. new_ref is passed in install_branch_config() in latest code. I guess you already made sure this function did not make any assumption about new_ref's length? The function install_branch_config uses the strbuf, as I wrote in the commit message. The contents of this buffer are then fed to git_config_set, which, after a few more function calls, parses the key with git_config_parse_key. This function does not rely on any assumptions (as far as I can tell!) on the name's length, and allocates enough space for it in https://github.com/git/git/blob/master/config.c#L1462. -- 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 2/2] fetch: handle overlaping refspecs on --prune
On Thu, 2014-02-27 at 12:19 -0800, Junio C Hamano wrote:
Carlos Martín Nieto c...@elego.de writes:
From: Carlos Martín Nieto c...@dwim.me
We need to consider that a remote-tracking branch may match more than
one rhs of a fetch refspec.
Hmph, do we *need* to, really?
Do you mean fetching one ref on the remote side and storing that in
multiple remote-tracking refs on our side? What benefit does such
an arrangement give the user? When we git fetch $there $that_ref
No, I mean a different kind of overlap, where the right-hand side
matches more refs than appear on the left side. In this particular case,
we would have something like
refs/heads/*:refs/remotes/origin/*
refs/pull/*/head:refs/remotes/origin/pr/*
as fetch refspecs. Going remote - remote-tracking branch is not an
issue, as each remote head only matches one refspec. However, we now
have 'origin/master' and 'origin/pr/5' both of which match the
'refs/remotes/origin/*' pattern. The current behaviour is to stop at the
first match, which would mark it as stale as there is no
'refs/heads/pr/5' branch in the remote.
In lieu of real namespacing support for remotes, this seems like a
reasonable way of coalescing the namespaces in the remote repo. I'll
update the commit message with more exact explanation of what kind of
overlap we're dealing with, as it seems it could do with help. Is there
maybe a better word to describe this setup than overlapping?
to obtain that single ref, do we update both remote-tracking refs?
When the user asks git log $that_ref@{upstream}, which one of two
or more remote-tracking refs should we consult? Should we report
an error if these remote-tracking refs that are supposed to track
the same remote ref not all match? Does git push $there $that_ref
to update that remote ref update all of these remote-tracking refs
on our side? Should it?
My knee-jerk reaction is that it may not be worth supporting such an
arrangement as broken (we may even want to diagnose it as an error),
but assuming we do need to, the approach to solve it, i.e. this...
For this (other) situation, where you duplicate refs, the issue we're
dealing with in these patches wouldn't arise. I have argued similarly
against built-in support in libgit2 for this kind of shenanigans, but
apparently there's people who use it, though their motivations remain a
mystery to me. Luckily we can support *that* quite well by just going
through the refspecs one by one and applying the rules (both in git and
libgit2).
cmn
In such a case, it is not enough to stop at
the first match but look at all of the matches in order to determine
whether a head is stale.
... sounds sensible.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()
On Fri, Feb 28, 2014 at 4:40 PM, Dmitry S. Dolzhenko
dmitrys.dolzhe...@yandex.ru wrote:
Signed-off-by: Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru
---
builtin/pack-objects.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..56a6fc8 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1213,12 +1213,7 @@ static int check_pbase_path(unsigned hash)
if (0 = pos)
return 1;
pos = -pos - 1;
- if (done_pbase_paths_alloc = done_pbase_paths_num) {
- done_pbase_paths_alloc = alloc_nr(done_pbase_paths_alloc);
- done_pbase_paths = xrealloc(done_pbase_paths,
- done_pbase_paths_alloc *
- sizeof(unsigned));
- }
+ ALLOC_GROW(done_pbase_paths, done_pbase_paths_num + 1,
done_pbase_paths_alloc);
Not strictly a rule, but I usually try to keep it within 80 columns,
unless the surrounding code already breaks it.
done_pbase_paths_num++;
If you move this up one line, then you don't have to + 1 in ALLOC_GROW
if (pos done_pbase_paths_num)
memmove(done_pbase_paths + pos + 1,
--
1.8.5.3
--
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
--
Duy
--
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 09/11] reflog-walk.c: use ALLOC_GROW() instead of inline code
On Fri, Feb 28, 2014 at 4:46 PM, Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru wrote: Affected functions: read_one_reflog(), add_commit_info() We can usually see this from @@ line so it's not really needed to describe. Same comment for a few other patches. -- Duy -- 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 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()
On Fri, Feb 28, 2014 at 7:32 PM, Duy Nguyen pclo...@gmail.com wrote: done_pbase_paths_num++; If you move this up one line, then you don't have to + 1 in ALLOC_GROW same comment to a few other patches. The rest of your series looks good. -- Duy -- 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] branch.c: delete size check of newly tracked branch names
On Fri, Feb 28, 2014 at 7:14 PM, Jacopo Notarstefano jacopo.notarstef...@gmail.com wrote: Nice. new_ref is passed in install_branch_config() in latest code. I guess you already made sure this function did not make any assumption about new_ref's length? The function install_branch_config uses the strbuf, as I wrote in the commit message. The contents of this buffer are then fed to git_config_set, which, after a few more function calls, parses the key with git_config_parse_key. This function does not rely on any assumptions (as far as I can tell!) on the name's length, and allocates enough space for it in https://github.com/git/git/blob/master/config.c#L1462. Thanks for checking! -- Duy -- 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: GSoC idea: allow git rebase --interactive todo lines to take options
On Thu, Feb 27, 2014 at 01:10:30PM -0500, Brandon McCaig wrote: On Wed, Feb 26, 2014 at 5:52 AM, Jeff King p...@peff.net wrote: This seems like a reasonable feature to me. All of your examples are possible with an edit and another git command, but the convenience may be worth it (though personally, most of the examples you gave are particularly interesting to me[1]). This strikes me as over-complicating the rebase --interactive interface. Sorry, I missed an important word in my final sentence. It should have been the examples you gave are NOT particularly interesting to me. Particularly all of the ideas expressed later on about merge commits and resetting authors, etc. It seems like you're trying to define a whole new command set (i.e., API) for Git, but within the context of rebase --interactive. I think it would be hard to document this, and hard to learn it, and harder still to remember it (even though it would obviously try to mirror the existing Git command API). I agree some of the examples are getting esoteric. Things like --signoff and --reset-author are a fairly straightforward convenience feature: they save you from writing exec git commit --amend --signoff. For others that cannot currently be done with a simple option to git commit, I think a reasonable first step would be to implement them there. For example, you cannot currently git commit --tree. Maybe that is too insane and low-level an option for git commit. But if it is, then it is almost certainly too insane and low-level for a rebase instruction. For others from Michael's list, I expect they may not make _sense_ outside of a rebase. That is, they are operations whose input is not a single commit, but a sequence of commits (e.g., if you had some high-level command that allowed swapping two commits without having to redo the conflicts from the second commit). Those ones might make sense to exist as part of rebase and nowhere else (but then they are not necessarily just options, but rather new instructions). That said, I do think that this is probably a bad direction and shouldn't be rushed into too fast. I'm not sure whether it is a good idea or not. But I think it is looking decreasingly like a good GSoC project. -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] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname
On Fri, Feb 28, 2014 at 4:46 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Fri, Feb 28, 2014 at 3:28 AM, Sun He sunheeh...@gmail.com wrote:
Signed-off-by: Sun He sunheeh...@gmail.com
---
Nicely done.
Due to the necessary changes to finish_tmp_packfile(), the focus of
this patch has changed slightly from that suggested by the
microproject. It's really now about finish_tmp_packfile() rather than
finish_bulk_checkin(). As such, it may make sense to adjust the patch
subject to reflect this. For instance:
Subject: finish_tmp_packfile(): use strbuf for pathname construction
You may also want your commit message to explain why you chose this
approach over other legitimate approaches. For instance, your change
places extra burden on callers of finish_tmp_packfile() by leaking a
detail of its implementation: namely that it wants to manipulate
pathnames easily (via strbuf). An equally valid and more encapsulated
approach would be for finish_tmp_packfile() to accept a 'const char *'
and construct its own strbuf internally.
If the extra strbuf creation in the alternate approach is measurably
slower, then you could use that fact to justify your implementation
choice in the commit message. On the other hand, if it's not
measurably slower, then perhaps the more encapsulated approach with
cleaner API might be preferable.
More comments below.
] diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..72fb82b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -803,7 +803,7 @@ static void write_pack_file(void)
if (!pack_to_stdout) {
struct stat st;
- char tmpname[PATH_MAX];
+ struct strbuf tmpname = STRBUF_INIT;
/*
* Packs are runtime accessed in their mtime
@@ -823,26 +823,22 @@ static void write_pack_file(void)
utb.modtime = --last_mtime;
if (utime(pack_tmp_name, utb) 0)
warning(failed utime() on %s: %s,
- tmpname, strerror(errno));
+ pack_tmp_name,
strerror(errno));
Good catch finding this bug (as your commentary below states).
Ideally, each conceptual change should be presented distinctly from
others, so this bug should have its own patch (even though it's just a
one-liner).
}
- /* Enough space for -sha-1.pack? */
- if (sizeof(tmpname) = strlen(base_name) + 50)
- die(pack base name '%s' too long,
base_name);
- snprintf(tmpname, sizeof(tmpname), %s-, base_name);
+ strbuf_addf(tmpname, %s-, base_name);
if (write_bitmap_index) {
bitmap_writer_set_checksum(sha1);
bitmap_writer_build_type_index(written_list,
nr_written);
}
- finish_tmp_packfile(tmpname, pack_tmp_name,
+ finish_tmp_packfile(tmpname, pack_tmp_name,
written_list, nr_written,
pack_idx_opts, sha1);
if (write_bitmap_index) {
- char *end_of_name_prefix = strrchr(tmpname,
0);
- sprintf(end_of_name_prefix, %s.bitmap,
sha1_to_hex(sha1));
+ strbuf_addf(tmpname, %s.bitmap
,sha1_to_hex(sha1));
stop_progress(progress_state);
diff --git a/pack-write.c b/pack-write.c
index 9b8308b..2204aa9 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char
**pack_tmp_name)
return sha1fd(fd, *pack_tmp_name);
}
-void finish_tmp_packfile(char *name_buffer,
+void finish_tmp_packfile(struct strbuf *name_buffer,
const char *pack_tmp_name,
struct pack_idx_entry **written_list,
uint32_t nr_written,
@@ -344,7 +344,7 @@ void finish_tmp_packfile(char *name_buffer,
unsigned char sha1[])
{
const char *idx_tmp_name;
- char *end_of_name_prefix = strrchr(name_buffer, 0);
+ int pre_len = name_buffer-len;
Minor: Perhaps basename_len (or some such) would convey a bit more
meaning than pre_len.
if (adjust_shared_perm(pack_tmp_name))
die_errno(unable to make temporary pack file readable);
@@ -354,17 +354,21 @@ void finish_tmp_packfile(char *name_buffer,
if (adjust_shared_perm(idx_tmp_name))
die_errno(unable to make temporary index file readable);
- sprintf(end_of_name_prefix,
Re: [PATCH 17/19] Portable alloca for Git
On Mon, Feb 24, 2014 at 5:21 PM, Kirill Smelkov k...@mns.spb.ru wrote: diff --git a/Makefile b/Makefile index dddaf4f..0334806 100644 --- a/Makefile +++ b/Makefile @@ -316,6 +321,7 @@ endif ifeq ($(uname_S),Windows) GIT_VERSION := $(GIT_VERSION).MSVC pathsep = ; + HAVE_ALLOCA_H = YesPlease NO_PREAD = YesPlease NEEDS_CRYPTO_WITH_SSL = YesPlease NO_LIBGEN_H = YesPlease In MSVC, alloca is defined in in malloc.h, not alloca.h: http://msdn.microsoft.com/en-us/library/wb1s57t5.aspx In fact, it has no alloca.h at all. But we don't have malloca.h in mingw either, so creating a compat/win32/alloca.h that includes malloc.h is probably sufficient. -- 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 17/19] Portable alloca for Git
On Fri, Feb 28, 2014 at 2:44 PM, Erik Faye-Lund kusmab...@gmail.com wrote: On Mon, Feb 24, 2014 at 5:21 PM, Kirill Smelkov k...@mns.spb.ru wrote: diff --git a/Makefile b/Makefile index dddaf4f..0334806 100644 --- a/Makefile +++ b/Makefile @@ -316,6 +321,7 @@ endif ifeq ($(uname_S),Windows) GIT_VERSION := $(GIT_VERSION).MSVC pathsep = ; + HAVE_ALLOCA_H = YesPlease NO_PREAD = YesPlease NEEDS_CRYPTO_WITH_SSL = YesPlease NO_LIBGEN_H = YesPlease In MSVC, alloca is defined in in malloc.h, not alloca.h: http://msdn.microsoft.com/en-us/library/wb1s57t5.aspx In fact, it has no alloca.h at all. But we don't have malloca.h in mingw either, so creating a compat/win32/alloca.h that includes malloc.h is probably sufficient. But we don't have alloca.h in mingw either, sorry. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream
On Fri, Feb 28, 2014 at 08:16:13PM +0900, Brian Gesiak wrote: I just don't want to regress somebody else's workflow due to my lack of imagination. This makes a lot of sense to me, although as-is the function emits a warning and returns immediately (although with a successful status code), so I'm also stumped as to what kind of workflow this would be included in. I'm cc-ing Matthieu, who wrote 85e2233, which introduces the check. Its commit message says: branch: warn and refuse to set a branch as a tracking branch of itself. Previous patch allows commands like git branch --set-upstream foo foo, which doesn't make much sense. Warn the user and don't change the configuration in this case. Don't die to let the caller finish its job in such case. For those just joining us, we are focused on the final statement, and deciding whether we should die() in this case. But I am not clear what job it would want to be finishing (creating the branch, I guess, but you cannot be doing anything useful, since by definition the branch already exists and you are not changing its tip). There wasn't any relevant discussion on the list[1]. Matthieu, can you remember anything else that led to that decision? In any case, if the jury's out on this one, I suppose the two patches I submitted are good to merge? Part of me thinks the bump from warning to error belongs in its own patch anyway. Yeah, it should definitely be a separate patch on top. -Peff [1] Threads are: http://thread.gmane.org/gmane.comp.version-control.git/137297/focus=137299 and http://thread.gmane.org/gmane.comp.version-control.git/137401/focus=137403 but the discussion focused on the first part of the series. -- 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: Branch Name Case Sensitivity
Am 28.02.2014 07:41, schrieb Johannes Sixt: Am 2/28/2014 0:38, schrieb Lee Hopkins: If I understand the issue correctly, the problem is that packed-refs are always case-sensitive, even if core.ignorecase=true. OTOH, core.ignorecase is intended to affect filenames of the worktree, not anything else, BTW. from git-config(1): enables various workarounds to enable git to work better on filesystems that are not case sensitive It says nothing about work-tree only, so I'd expect it to apply to all git components that store potentially case-sensitive information in file names. ...it also says better, not flawlessly :-) checking / updating _unpacked_ refs on a case-insensitive file system is naturally case-insensitive. So wouldn't it be a better workaround to disallow packed refs (i.e. 'git config gc.packrefs false')? You are correct, the issue boils down to mixing the usage of packed-refs and loose refs on case insensitive file systems. So either always using packed-refs or always using loose refs would take care of the problem. Based Michael Haggerty's response, it seems that always using loose refs would be a better workaround. So, everybody on a case-insensitive file system should pay the price even if they do not need the feature? No way. If you are on a case-insensitive filesystem, or work on a cross-platform project, ensure that you avoid ambiguous refs. Problem solved. So its OK to lose data if you accidentally use an ambiguous ref? I cannot believe you actually meant that. IMO the proper solution is to teach packed-refs about core.ignorecase. Until that happens, disabling gc.packrefs seems to be a valid workaround for people who have that problem. -- 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: GSoC idea: allow git rebase --interactive todo lines to take options
On 02/28/2014 01:52 PM, Jeff King wrote: [...] Sorry, I missed an important word in my final sentence. It should have been the examples you gave are NOT particularly interesting to me. On Thu, Feb 27, 2014 at 01:10:30PM -0500, Brandon McCaig wrote: Particularly all of the ideas expressed later on about merge commits and resetting authors, etc. It seems like you're trying to define a whole new command set (i.e., API) for Git, but within the context of rebase --interactive. I think it would be hard to document this, and hard to learn it, and harder still to remember it (even though it would obviously try to mirror the existing Git command API). I agree some of the examples are getting esoteric. Things like --signoff and --reset-author are a fairly straightforward convenience feature: they save you from writing exec git commit --amend --signoff. For others that cannot currently be done with a simple option to git commit, I think a reasonable first step would be to implement them there. For example, you cannot currently git commit --tree. Maybe that is too insane and low-level an option for git commit. But if it is, then it is almost certainly too insane and low-level for a rebase instruction. For others from Michael's list, I expect they may not make _sense_ outside of a rebase. That is, they are operations whose input is not a single commit, but a sequence of commits (e.g., if you had some high-level command that allowed swapping two commits without having to redo the conflicts from the second commit). Those ones might make sense to exist as part of rebase and nowhere else (but then they are not necessarily just options, but rather new instructions). That said, I do think that this is probably a bad direction and shouldn't be rushed into too fast. I'm not sure whether it is a good idea or not. But I think it is looking decreasingly like a good GSoC project. I guess I misread the sentiment of the mailing list, because I merged this idea into the list about two hours ago. I'm not claiming that all of the sub-ideas are good, but I do think that some of them are, and that the general idea of allowing options on todo-list commands would make it possible for them to be more expressive while *avoiding* making them a lot harder to learn. I would rather give the user a few options that can be used consistently on multiple commands than have to invent a new command for each new feature. And I think that the line-oriented nature of the todo list makes pick --signoff 1234abc Blah blah easier to understand (and easier to type) than pick 1234abc Blah blah amend --signoff let alone pick 1234abc Blah blah exec git commit --amend --signoff I also like the idea of a non-broken git rebase --interactive --preserve-merges via a todo option -p or something similar. But if you think that even the proposal's simpler sub-ideas are controversial, then let me know and I will delete the idea from the list again. I don't want a GSoC student to have to fight battles of my own creation :-) 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: Branch Name Case Sensitivity
If you are on a case-insensitive filesystem, or work on a cross-platform project, ensure that you avoid ambiguous refs. Problem solved. I agree this is the best solution, and I personally avoid the use of ambiguous refs. However, since there is nothing in git stopping the use of ambiguous refs, there is no way to stop every person who works on a shared repo from using them. So, everybody on a case-insensitive file system should pay the price even if they do not need the feature? No way. I would say preventing potential loss of commits is a price worth paying. IMO the proper solution is to teach packed-refs about core.ignorecase. Until that happens, disabling gc.packrefs seems to be a valid workaround for people who have that problem. Once again, based on Michael Haggerty's very informative input, maybe an even better solution would be to add a core.allowambiguousrefs (default to true) option and when it is false do a case insensitive comparison during ref creation (branching, tagging). Thanks, -Lee -- 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: [RFC 0/3] Make git more user-friendly during a merge conflict
David Kastrup d...@gnu.org writes: Stephen Leake stephen_le...@stephe-leake.org writes: I like commands that do the right thing. So no, this would not be confusing. I _hate_ commands that think they know better than to do what they are told. In particular when doing destructive things. And just because _you_ like them does not mean they are not confusing. Ok, I should have said not confusing for me. People differ. In the long run, it is much more confusing if you come to rely on some commands doing the right thing while in other cases, the actually written thing is done. There should always be the option of telling git exactly what to do. In my emacs front end, the command that does the right thing is _called_ do-the-right-thing. All of the other commands do exactly as told. In this case, it is only git reset that would do the right thing, since you did _not_ tell it specifically what to do. Relying on a default is always problematic, in my experience; I much prefer no default to a default that people voted on 10 years ago, and now we are stuck with it. do the right thing commands also tend to do the wrong thing occasionally with potentially disastrous results when they are used in scripts where the followup actions rely on the actual result. That is bad, and should not be allowed. On the other hand, I have yet to see an actual use case of bad behavior in this discussion. -- -- 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: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname
2014-02-28 21:12 GMT+08:00 Eric Sunshine [via git]
ml-node+s661346n760450...@n2.nabble.com:
On Fri, Feb 28, 2014 at 4:46 AM, Eric Sunshine [hidden email] wrote:
On Fri, Feb 28, 2014 at 3:28 AM, Sun He [hidden email] wrote:
Signed-off-by: Sun He [hidden email]
---
Nicely done.
Due to the necessary changes to finish_tmp_packfile(), the focus of
this patch has changed slightly from that suggested by the
microproject. It's really now about finish_tmp_packfile() rather than
finish_bulk_checkin(). As such, it may make sense to adjust the patch
subject to reflect this. For instance:
Subject: finish_tmp_packfile(): use strbuf for pathname construction
You may also want your commit message to explain why you chose this
approach over other legitimate approaches. For instance, your change
places extra burden on callers of finish_tmp_packfile() by leaking a
detail of its implementation: namely that it wants to manipulate
pathnames easily (via strbuf). An equally valid and more encapsulated
approach would be for finish_tmp_packfile() to accept a 'const char *'
and construct its own strbuf internally.
If the extra strbuf creation in the alternate approach is measurably
slower, then you could use that fact to justify your implementation
choice in the commit message. On the other hand, if it's not
measurably slower, then perhaps the more encapsulated approach with
cleaner API might be preferable.
Thank you for your explaination. I get the point.
And I think if it is proved that the strbuf way is measurably slower.
We should add a check for the length of string before we sprintf().
More comments below.
] diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..72fb82b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -803,7 +803,7 @@ static void write_pack_file(void)
if (!pack_to_stdout) {
struct stat st;
- char tmpname[PATH_MAX];
+ struct strbuf tmpname = STRBUF_INIT;
/*
* Packs are runtime accessed in their mtime
@@ -823,26 +823,22 @@ static void write_pack_file(void)
utb.modtime = --last_mtime;
if (utime(pack_tmp_name, utb) 0)
warning(failed utime() on %s:
%s,
- tmpname,
strerror(errno));
+ pack_tmp_name,
strerror(errno));
Good catch finding this bug (as your commentary below states).
Ideally, each conceptual change should be presented distinctly from
others, so this bug should have its own patch (even though it's just a
one-liner).
}
- /* Enough space for -sha-1.pack? */
- if (sizeof(tmpname) = strlen(base_name) + 50)
- die(pack base name '%s' too long,
base_name);
- snprintf(tmpname, sizeof(tmpname), %s-,
base_name);
+ strbuf_addf(tmpname, %s-, base_name);
if (write_bitmap_index) {
bitmap_writer_set_checksum(sha1);
bitmap_writer_build_type_index(written_list, nr_written);
}
- finish_tmp_packfile(tmpname, pack_tmp_name,
+ finish_tmp_packfile(tmpname, pack_tmp_name,
written_list, nr_written,
pack_idx_opts, sha1);
if (write_bitmap_index) {
- char *end_of_name_prefix =
strrchr(tmpname, 0);
- sprintf(end_of_name_prefix, %s.bitmap,
sha1_to_hex(sha1));
+ strbuf_addf(tmpname, %s.bitmap
,sha1_to_hex(sha1));
stop_progress(progress_state);
diff --git a/pack-write.c b/pack-write.c
index 9b8308b..2204aa9 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char
**pack_tmp_name)
return sha1fd(fd, *pack_tmp_name);
}
-void finish_tmp_packfile(char *name_buffer,
+void finish_tmp_packfile(struct strbuf *name_buffer,
const char *pack_tmp_name,
struct pack_idx_entry **written_list,
uint32_t nr_written,
@@ -344,7 +344,7 @@ void finish_tmp_packfile(char *name_buffer,
unsigned char sha1[])
{
const char *idx_tmp_name;
- char *end_of_name_prefix = strrchr(name_buffer, 0);
+ int pre_len = name_buffer-len;
Minor: Perhaps basename_len (or some such) would convey a bit more
meaning than pre_len.
if (adjust_shared_perm(pack_tmp_name))
die_errno(unable to make temporary pack file
Re: [PATCH v2 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()
On 02/28/2014 01:40 PM, Duy Nguyen wrote: On Fri, Feb 28, 2014 at 7:32 PM, Duy Nguyen pclo...@gmail.com wrote: done_pbase_paths_num++; If you move this up one line, then you don't have to + 1 in ALLOC_GROW same comment to a few other patches. The rest of your series looks good. Duy, The example in Documentation/technical/api-allocation-growing.txt does it the same way as Dmitry: ALLOC_GROW(item, nr + 1, alloc); item[nr++] = value you like; The alternative, nr++; ALLOC_GROW(item, nr, alloc); item[nr] = value you like; is an extra line, which is at least a small argument for the variant shown in the docs. (Since ALLOC_GROW is a macro, it is not OK to use ++nr as its second argument.) Personally, I also prefer the shorter version. The line item[nr++] = value is an easy-to-recognize idiom, and ALLOC_GROW(item, nr + 1, alloc); somehow makes it more transparent by how much more space will be needed. So my vote is that the patches are OK the way Dmitry wrote them (mind, I have only read through 05/11 so far). 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: [RFC 0/3] Make git more user-friendly during a merge conflict
Stephen Leake stephen_le...@stephe-leake.org writes: David Kastrup d...@gnu.org writes: do the right thing commands also tend to do the wrong thing occasionally with potentially disastrous results when they are used in scripts where the followup actions rely on the actual result. That is bad, and should not be allowed. On the other hand, I have yet to see an actual use case of bad behavior in this discussion. Huh. http://permalink.gmane.org/gmane.comp.version-control.git/242744 -- David Kastrup -- 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 GSoC idea: new git config features
I just wrote up another double-idea that has been stewing in my head for a while: * Allow configuration values to be unset via a config file * Fix git config --unset to clean up detritus from sections that are left empty. These ideas are more out there than the last, and might be too controversial to be implemented, let alone as a GSoC project. I'd definitely like some feedback. And if you like this idea or the other one I proposed, please volunteer to be a co-mentor! I will be traveling for a few weeks this summer, so I *won't* be able to be the sole mentor to a student. I wrote up this idea in the following pull request: https://github.com/git/git.github.io/pull/6 I will also append the text, for your mailing-list-reading convenience. Michael ### `git config` improvements This project proposes the following two improvements to `git config`. Please note that this project has a significant political component to it, because some of the details of the features will be controversial. Unsetting configuration options Some Git configuration options have an effect by their mere existence. (I.e., setting the option to false or the empty string is different than leaving it unset altogether.) Also, some configuration options can take multiple values. However, there is no way for an option file to unset an option--that is, to change the option back to unset. This is awkward, because configuration values are read from various places (`/etc/gitconfig`, `~/.config/git/config` or `~/.gitconfig`, and `$GIT_DIR/config`, plus perhaps files that are included by other configuration files). Therefore, if an option is set in one of the earlier files, there is no way for it to be unset in a later one. The unwanted option might have even been set in a file like `/etc/gitconfig` that the user doesn't have permission to modify. It would be nice to have a syntax that can be used to unset any previously-defined values for an option. Perhaps [section subsection] !option The above is currently currently a syntax error that causes Git to terminate, so some thought has to go into a transition plan for enabling this feature. Maybe a syntax has to be invented that conforms to the current format, like [unset] name = section.subsection.option Because options are currently processed as they are read, this change will require the code that reads options files to be changed significantly. Leave yourself a lot of time to attain a consensus on the mailing list about how this can be done while retaining reasonable backwards compatibility. Tidy configuration files When a configuration file is repeatedly modified, often garbage is left behind. For example, after git config my.option true git config --unset my.option git config my.option true git config --unset my.option the bottom of the configuration file is left with the useless lines [my] [my] It would be nice to clean up such garbage when rewriting the configuration file. This project is a bit tricky because of the possible presence of comments. For example, what if an empty section looks like this: [my] # This section is for my own private settings or this: [my] # This section is for my own private settings or this: # This section is for my own private settings: [my] ? In some such cases it might be desireable either to retain the section even though it is empty, or to delete the comment along with the section. Very likely there will be some obvious patterns when everybody agrees that an empty section can be deleted, and other, more controversial cases where you will have to reach a consensus on the mailing list about what should be done. - Language: C - Difficulty: medium - Possible mentors: Michael Haggerty -- 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: Branch Name Case Sensitivity
On Fri, Feb 28, 2014 at 4:13 PM, Michael Haggerty mhag...@alum.mit.edu wrote: On 02/28/2014 12:38 AM, Lee Hopkins wrote: [...] Based Michael Haggerty's response, it seems that always using loose refs would be a better workaround. No, I answered the question what would be the disadvantages of using only packed refs?. Now I will answer the question what would be the disadvantages of using only loose refs?: 1. Efficiency. Any time all of the references have to be read, loose refs are far slower than packed refs. 2. Disk space and inode usage: loose refs consume one inode and one disk sector (typically 4k) each, whereas packed refs consume only one inode in total, and many packed refs can fit into each disk sector. After all, there is a reason that we have both packed refs and loose refs. The basic idea is to use packed refs for the bulk of references, especially cold references like tags that only change infrequently, but to store hot references as loose refs so that they can be modified cheaply. Could we have a staging place for new refs in between? Case sensitivity is just another limitation we hit because we rely on filesystem. We already have problems with having both refs foo and foo/bar at the same time. Not all repos are super busy and need the top efficiencies of loose refs. And about rewriting packed-refs every time, I don't think that's a big problem for normal repos. linux-2.6 index file is 4MB(*) and it's rewritten on nearly every worktree-related operation and nobody complains (out loud anyway). Assuming an average ref takes 100 bytes, that's about 41k refs. (*) it's 3MB with index-v4 but I don't think v4 is popular -- Duy -- 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] OPTION_CMDMODE should be used when not accept an argument, and OPTION_NUMBER is of special type. So change the mode to OPTION_CMDMODE
Signed-off-by: Sun He sunheeh...@gmail.com --- parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index 7b8d3fa..59a52b0 100644 --- a/parse-options.c +++ b/parse-options.c @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts) case OPTION_NEGBIT: case OPTION_SET_INT: case OPTION_SET_PTR: - case OPTION_NUMBER: + case OPTION_CMDMODE: if ((opts-flags PARSE_OPT_OPTARG) || !(opts-flags PARSE_OPT_NOARG)) err |= optbug(opts, should not accept an argument); -- 1.9.0.138.g2de3478.dirty --- I came across this protential bug. According to parse-options.h OPTION_CMDMODE is an option with noarguments and OPTION_NUMBER is special type option. Thanks, He Sun -- 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] OPTION_CMDMODE should be used when not accept an argument, and OPTION_NUMBER is of special type. So change the mode to OPTION_CMDMODE
I am not sure if this is a bug. I need your help to find out it. Cheers, He Sun 2014-02-28 22:29 GMT+08:00 Sun He sunheeh...@gmail.com: Signed-off-by: Sun He sunheeh...@gmail.com --- parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index 7b8d3fa..59a52b0 100644 --- a/parse-options.c +++ b/parse-options.c @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts) case OPTION_NEGBIT: case OPTION_SET_INT: case OPTION_SET_PTR: - case OPTION_NUMBER: + case OPTION_CMDMODE: if ((opts-flags PARSE_OPT_OPTARG) || !(opts-flags PARSE_OPT_NOARG)) err |= optbug(opts, should not accept an argument); -- 1.9.0.138.g2de3478.dirty --- I came across this protential bug. According to parse-options.h OPTION_CMDMODE is an option with noarguments and OPTION_NUMBER is special type option. Thanks, He Sun -- 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 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()
On Fri, Feb 28, 2014 at 9:20 PM, Michael Haggerty mhag...@alum.mit.edu wrote: Duy, The example in Documentation/technical/api-allocation-growing.txt does it the same way as Dmitry: ALLOC_GROW(item, nr + 1, alloc); item[nr++] = value you like; The alternative, nr++; ALLOC_GROW(item, nr, alloc); item[nr] = value you like; is an extra line, which is at least a small argument for the variant shown in the docs. (Since ALLOC_GROW is a macro, it is not OK to use ++nr as its second argument.) Personally, I also prefer the shorter version. The line item[nr++] = value is an easy-to-recognize idiom, and ALLOC_GROW(item, nr + 1, alloc); somehow makes it more transparent by how much more space will be needed. So my vote is that the patches are OK the way Dmitry wrote them (mind, I have only read through 05/11 so far). I'm not saying all patches should do nr++; ALLOC_GROW(item, nr, alloc); only those that do if (..) realloc...; nr++; should be reordered. Those changes that do item[nr++] = yyy should be kept. Anyway it's just an observation, not something that should block these patches. -- Duy -- 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: Branch Name Case Sensitivity
On 02/28/2014 03:31 PM, Duy Nguyen wrote: On Fri, Feb 28, 2014 at 4:13 PM, Michael Haggerty mhag...@alum.mit.edu wrote: On 02/28/2014 12:38 AM, Lee Hopkins wrote: [...] Based Michael Haggerty's response, it seems that always using loose refs would be a better workaround. No, I answered the question what would be the disadvantages of using only packed refs?. Now I will answer the question what would be the disadvantages of using only loose refs?: 1. Efficiency. Any time all of the references have to be read, loose refs are far slower than packed refs. 2. Disk space and inode usage: loose refs consume one inode and one disk sector (typically 4k) each, whereas packed refs consume only one inode in total, and many packed refs can fit into each disk sector. After all, there is a reason that we have both packed refs and loose refs. The basic idea is to use packed refs for the bulk of references, especially cold references like tags that only change infrequently, but to store hot references as loose refs so that they can be modified cheaply. Could we have a staging place for new refs in between? Case sensitivity is just another limitation we hit because we rely on filesystem. We already have problems with having both refs foo and foo/bar at the same time. Not all repos are super busy and need the top efficiencies of loose refs. True. Nor should most people usually need the ability to run multiple git commands simultaneously. In fact, I've started working on a pluggable backend for reference storage. After that change, it should be easy to experiment with different combinations of loose-only, packed-only, or other (new) storage schemes that don't suffer from directory/file conflicts, etc. I haven't talked about this work on the list yet because it's still very young. 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] OPTION_CMDMODE should be used when not accept an argument, and OPTION_NUMBER is of special type. So change the mode to OPTION_CMDMODE
Way too long subject line. Keep it within 70-75 chars. The rest could be put in the body. On Fri, Feb 28, 2014 at 9:32 PM, 孙赫 sunheeh...@gmail.com wrote: I am not sure if this is a bug. I need your help to find out it. Tip:git has a wonderful history (most of it anyway). Try git log --patch parse-options.[ch] to understand parse-options evolution. Add -SOPTION_NUMBER (or -SOPTION_CMDMODE) to limit to only commits whose diff contains that keyword. -- Duy -- 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 00/11] Use ALLOC_GROW() instead of inline code
On 02/28/2014 10:29 AM, Dmitry S. Dolzhenko wrote: Thank you for your remarks. In this patch I tried to take them into account. Dmitry S. Dolzhenko (11): builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW() bundle.c: change add_to_ref_list() to use ALLOC_GROW() cache-tree.c: change find_subtree() to use ALLOC_GROW() commit.c: change register_commit_graft() to use ALLOC_GROW() diff.c: use ALLOC_GROW() instead of inline code diffcore-rename.c: use ALLOC_GROW() instead of inline code patch-ids.c: change add_commit() to use ALLOC_GROW() replace_object.c: change register_replace_object() to use ALLOC_GROW() reflog-walk.c: use ALLOC_GROW() instead of inline code dir.c: change create_simplify() to use ALLOC_GROW() attr.c: change handle_attr_line() to use ALLOC_GROW() attr.c | 7 +-- builtin/pack-objects.c | 7 +-- bundle.c | 6 +- cache-tree.c | 6 +- commit.c | 8 ++-- diff.c | 12 ++-- diffcore-rename.c | 12 ++-- dir.c | 5 + patch-ids.c| 5 + reflog-walk.c | 13 +++-- replace_object.c | 8 ++-- 11 files changed, 17 insertions(+), 72 deletions(-) Everything looks fine to me. Assuming the test suite ran 100%, Acked-by: Michael Haggerty mhag...@alum.mit.edu Thanks! 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 1/2] t3200-branch: test setting branch as own upstream
Jeff King p...@peff.net writes: Don't die to let the caller finish its job in such case. [...] Matthieu, can you remember anything else that led to that decision? Not at all, unfortunately. I don't remember if I did that in case there's something like some cleanup to do or because I had something more precise in mind. A case to be carefull about is if you're using the same git branch command for multiple actions (trying --set-upstream in combination with other options). But I do not see a case where this would be possible. -- 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 stash pop` UX Problem
Brandon McCaig bamcc...@gmail.com writes: On Thu, Feb 27, 2014 at 9:57 PM, Stephen Leake stephen_le...@stephe-leake.org wrote: You might be adding other files for other reasons. But if you add a file that does resolve a conflict caused by 'git stash pop', it is not guessing. Staging a file doesn't tell git that you resolved a conflict. Git will happily accept a blob full of conflict markers. Git doesn't know the difference. Git expects the user to know what is right. The user has the freedom to manipulate the index as they see fit, which means both adding and removing from it anytime they wish. But git has a notion of unresolved conflict. For example, when I have conflicts from a 'git stash pop', 'git status' shows: stephe@takver$ git status # On branch master # Unmerged paths: # (use git reset HEAD file... to unstage) # (use git add/rm file... as appropriate to mark resolution) # # both modified: CommandBasedAutonomous.java # both modified: DriveByInches.java # # ... How does it know those files are unmerged? I'm guessing it has recorded the fact that they had conflicts. Where does it record that? In fact, at this point, I have edited CommandBasedAutonomous.java to resolve the conflicts. But git apparently doesn't know that. So I do 'git add CommandBasedAutonomous.java', then 'git status': stephe@takver$ git status # On branch master # Changes to be committed: # (use git reset HEAD file... to unstage) # # modified: AerialAssist2014/src/org/usfirst/frc1939/AerialAssist2014/commands/CommandBasedAutonomous.java # # Unmerged paths: # (use git reset HEAD file... to unstage) # (use git add/rm file... as appropriate to mark resolution) # # both modified: AerialAssist2014/src/org/usfirst/frc1939/AerialAssist2014/commands/DriveByInches.java And git thinks that file is now merged. So it appears that adding a file _does_ tell git that the conflict is resolved. Or am I still missing something? So git add and git stash * are lower level tools; to get the effect we are asking for, we should use a front-end (which is why I'm writing one for Emacs :). You *can* use a front end, but I would argue that you shouldn't necessarily. Most third-party front ends only serve to confuse users. In general, they only cause problems and encourage ignorance. Won't happen here; I'm writing it. It may confuse other people, but not me :). Git is a very pure system. Hmm. We'll have to disagree on that. git gives the impression of having grown organically for quite a while, and therefore suffers from changing and competing design paradigms and conflicting requirements due to preserving backward compatiblity. monotone is much cleaner, since it has had very few design paradigm changes, and they were implemented cleanly, without preserving backward compatibility. monotone is not as flexible as git, but what I've seen so far could be added to monotone (I don't think it ever will be; monotone is dying as a project). We are probably using different definitions of pure here. It is up to the user to learn how to assemble those tools for good (and many front ends exist to help; sometimes arguably too many as it is, such as git-pull(1) for example). Yes. Which is why we are discussing how much help git should be while still learning the rules. This isn't a case of the API being wrong. This is a case of PEBKAC, IMO. (wikipedia to the rescue; PEBKAC = operator error) Yes, I'm not using it correctly, because I don't understand it yet. That's the definition of newbie. Dropping the stash after adding all changes to the index after a failed pop is not universal. Not universal, but it appears to be very common; it is certainly what I expect, as a newbie. So it could be the default as long as there is a configuration option to have it not do that. I _did_ RTFM (specifically the man page on 'git stash', and before that the git book at http://git-scm.com/documentation (which did not mention stash)); it did not explain the full cycle of how to resolve conflicts after stash pop. Perhaps there is a different manual that I could read instead? In particular, one that explains what unmerged paths means in the 'git status' output? The 'git-status' man page does not do that. -- -- 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: [PATCH] OPTION_CMDMODE should be used when not accept an argument, and OPTION_NUMBER is of special type. So change the mode to OPTION_CMDMODE
2014-02-28 22:43 GMT+08:00 Duy Nguyen [via git] ml-node+s661346n7604517...@n2.nabble.com: Way too long subject line. Keep it within 70-75 chars. The rest could be put in the body. On Fri, Feb 28, 2014 at 9:32 PM, 孙赫 [hidden email] wrote: I am not sure if this is a bug. I need your help to find out it. Tip:git has a wonderful history (most of it anyway). Try git log --patch parse-options.[ch] to understand parse-options evolution. Add -SOPTION_NUMBER (or -SOPTION_CMDMODE) to limit to only commits whose diff contains that keyword. -- Duy -- Got it, Thanks To unsubscribe from this list: send the line unsubscribe git in the body of a message to [hidden email] More majordomo info at http://vger.kernel.org/majordomo-info.html If you reply to this email, your message will be added to the discussion below: http://git.661346.n2.nabble.com/PATCH-OPTION-CMDMODE-should-be-used-when-not-accept-an-argument-and-OPTION-NUMBER-is-of-special-typeE-tp7604513p7604517.html To start a new topic under git, email ml-node+s661346n661346...@n2.nabble.com To unsubscribe from git, click here. NAML -- 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] OPTION_NUMBER should be replaced by OPTION_CMDMODE
Signed-off-by: Sun He sunheeh...@gmail.com --- parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index 7b8d3fa..59a52b0 100644 --- a/parse-options.c +++ b/parse-options.c @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts) case OPTION_NEGBIT: case OPTION_SET_INT: case OPTION_SET_PTR: - case OPTION_NUMBER: + case OPTION_CMDMODE: if ((opts-flags PARSE_OPT_OPTARG) || !(opts-flags PARSE_OPT_NOARG)) err |= optbug(opts, should not accept an argument); -- 1.9.0.138.g2de3478.dirty --- I came I came across this protential bug. According to parse-options.h OPTION_CMDMODE is an option with noarguments and OPTION_NUMBER is special type option Thanks, He Sun -- 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] builtin/pack-objects.c:write_pack_file() replace tmpname with pack_tmp_name in warning
Signed-off-by: Sun He sunheeh...@gmail.com --- builtin/pack-objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c733379..4922ce5 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -823,7 +823,7 @@ static void write_pack_file(void) utb.modtime = --last_mtime; if (utime(pack_tmp_name, utb) 0) warning(failed utime() on %s: %s, - tmpname, strerror(errno)); + pack_tmp_name, strerror(errno)); } /* Enough space for -sha-1.pack? */ -- 1.9.0.138.g2de3478.dirty --- The tmpname is uninitialized and it should a bug -- 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 stash pop` UX Problem
Stephen Leake stephen_le...@stephe-leake.org writes: So it appears that adding a file _does_ tell git that the conflict is resolved. Yes it does. Git _knows_ that you consider the conflict to be resolved. It cannot know how happy you are with the result. Similarly, in a conflicted merge, the last git add does not trigger a commit silently. And a silent commit would be much less serious than a silent data drop. -- 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
Fwd: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname
-- Forwarded message --
From: 孙赫 sunheeh...@gmail.com
Date: 2014-02-28 21:37 GMT+08:00
Subject: Re: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to
use a strbuf for handling packname
To: Eric Sunshine [via git] ml-node+s661346n7604473...@n2.nabble.com
2014-02-28 17:47 GMT+08:00 Eric Sunshine [via git]
ml-node+s661346n7604473...@n2.nabble.com:
On Fri, Feb 28, 2014 at 3:28 AM, Sun He [hidden email] wrote:
Signed-off-by: Sun He [hidden email]
---
Nicely done.
Due to the necessary changes to finish_tmp_packfile(), the focus of
this patch has changed slightly from that suggested by the
microproject. It's really now about finish_tmp_packfile() rather than
finish_bulk_checkin(). As such, it may make sense to adjust the patch
subject to reflect this. For instance:
Subject: finish_tmp_packfile(): use strbuf for pathname construction
That's great, I will adjust it as you suggested.
More comments below.
] diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..72fb82b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -803,7 +803,7 @@ static void write_pack_file(void)
if (!pack_to_stdout) {
struct stat st;
- char tmpname[PATH_MAX];
+ struct strbuf tmpname = STRBUF_INIT;
/*
* Packs are runtime accessed in their mtime
@@ -823,26 +823,22 @@ static void write_pack_file(void)
utb.modtime = --last_mtime;
if (utime(pack_tmp_name, utb) 0)
warning(failed utime() on %s:
%s,
- tmpname, strerror(errno));
+ pack_tmp_name,
strerror(errno));
Good catch finding this bug (as your commentary below states).
Ideally, each conceptual change should be presented distinctly from
others, so this bug should have its own patch (even though it's just a
one-liner).
OK. Should I send this patch?
}
- /* Enough space for -sha-1.pack? */
- if (sizeof(tmpname) = strlen(base_name) + 50)
- die(pack base name '%s' too long,
base_name);
- snprintf(tmpname, sizeof(tmpname), %s-,
base_name);
+ strbuf_addf(tmpname, %s-, base_name);
if (write_bitmap_index) {
bitmap_writer_set_checksum(sha1);
bitmap_writer_build_type_index(written_list, nr_written);
}
- finish_tmp_packfile(tmpname, pack_tmp_name,
+ finish_tmp_packfile(tmpname, pack_tmp_name,
written_list, nr_written,
pack_idx_opts, sha1);
if (write_bitmap_index) {
- char *end_of_name_prefix =
strrchr(tmpname, 0);
- sprintf(end_of_name_prefix, %s.bitmap,
sha1_to_hex(sha1));
+ strbuf_addf(tmpname, %s.bitmap
,sha1_to_hex(sha1));
stop_progress(progress_state);
diff --git a/pack-write.c b/pack-write.c
index 9b8308b..2204aa9 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char
**pack_tmp_name)
return sha1fd(fd, *pack_tmp_name);
}
-void finish_tmp_packfile(char *name_buffer,
+void finish_tmp_packfile(struct strbuf *name_buffer,
const char *pack_tmp_name,
struct pack_idx_entry **written_list,
uint32_t nr_written,
@@ -344,7 +344,7 @@ void finish_tmp_packfile(char *name_buffer,
unsigned char sha1[])
{
const char *idx_tmp_name;
- char *end_of_name_prefix = strrchr(name_buffer, 0);
+ int pre_len = name_buffer-len;
Minor: Perhaps basename_len (or some such) would convey a bit more
meaning than pre_len.
It's pretty good to work with you next suggestion. THX
if (adjust_shared_perm(pack_tmp_name))
die_errno(unable to make temporary pack file readable);
@@ -354,17 +354,21 @@ void finish_tmp_packfile(char *name_buffer,
if (adjust_shared_perm(idx_tmp_name))
die_errno(unable to make temporary index file readable);
- sprintf(end_of_name_prefix, %s.pack, sha1_to_hex(sha1));
- free_pack_by_name(name_buffer);
+ strbuf_addf(name_buffer, %s.pack, sha1_to_hex(sha1));
+ free_pack_by_name(name_buffer-buf);
- if (rename(pack_tmp_name, name_buffer))
+ if (rename(pack_tmp_name, name_buffer-buf))
die_errno(unable to rename temporary pack file);
-
RE: difftool sends malformed path to exernal tool on Windows
OK, so what can we do next? Paul -Original Message- From: Paul Lotz [mailto:pl...@lsst.org] Sent: Monday, February 24, 2014 9:44 AM To: 'David Aguilar' Cc: 'git@vger.kernel.org' Subject: RE: difftool sends malformed path to exernal tool on Windows David, Thanks for the helpful reply. As you suggested, I modified the .gitconfig file to have: [difftool test] cmd = echo \$LOCAL\ \$REMOTE\ and ran $ git difftool -t test An example of the the resulting console output is: C:/Users/Paul/AppData/Local/Temp/I8L2Bc_WriteTestParameters.vi Commands/StartAutomatedTest/WriteTestParameters.vi Paul -Original Message- From: David Aguilar [mailto:dav...@gmail.com] Sent: Friday, February 21, 2014 3:38 AM To: Paul Lotz Cc: git@vger.kernel.org Subject: Re: difftool sends malformed path to exernal tool on Windows On Mon, Feb 17, 2014 at 03:14:01PM -0700, Paul Lotz wrote: From the Git Bash command line, I enter $ git difftool and type ‘y’ when the file I want to difference appears. Git correctly calls the external diff tool (LVCompare.exe), but the path for the remote file Git passes to that tool is malformed (e.g., C:\/Users/Paul/AppData/Local/Temp/QCpqLa_calcLoadCellExcitation.vi). Obviously the \/ (backslash forwardslash) combination is incorrect. If this is the case then difftool is not the only one with this problem. We use the GIT_EXTERNAL_DIFF mechanism to run difftool under git diff, so it may be that the paths are mangled by git diff itself. I don't really know enough about msysgit to know for sure, though. What do you see if you create a dummy tool which just does echo? [difftool test] cmd = echo \$LOCAL\ \$REMOTE\ Then run: $ git difftool -t test For the record, I have successfully made calls to LVCompare.exe manually from a Windows command prompt directly (without Git). The relevant portion of the .gitconfig file is: [diff] tool = LVCompare [difftool LVCompare] cmd = 'C:/Program Files (x86)/National Instruments/Shared/LabVIEW Compare/LVCompare.exe' \$LOCAL\ \$REMOTE\ For the record, the operating system is Windows 8.1. Do any msysgit folks know whether GIT_EXTERNAL_DIFF is a known issue? -- David -- 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 stash pop` UX Problem
Stephen Leake stephen_le...@stephe-leake.org writes: Brandon McCaig bamcc...@gmail.com writes: On Thu, Feb 27, 2014 at 9:57 PM, Stephen Leake stephen_le...@stephe-leake.org wrote: You might be adding other files for other reasons. But if you add a file that does resolve a conflict caused by 'git stash pop', it is not guessing. Staging a file doesn't tell git that you resolved a conflict. Git will happily accept a blob full of conflict markers. Git doesn't know the difference. Git expects the user to know what is right. The user has the freedom to manipulate the index as they see fit, which means both adding and removing from it anytime they wish. But git has a notion of unresolved conflict. Not really. It has a notion of unmerged path. For example, when I have conflicts from a 'git stash pop', 'git status' shows: stephe@takver$ git status # On branch master # Unmerged paths: # (use git reset HEAD file... to unstage) # (use git add/rm file... as appropriate to mark resolution) # # both modified: CommandBasedAutonomous.java # both modified: DriveByInches.java # # ... How does it know those files are unmerged? I'm guessing it has recorded the fact that they had conflicts. Where does it record that? The index contains the unmerged versions of the file. Possibly also the version with conflict markers, but it's been too long since I last checked. After git add, there is only one version in the index. If you apply a stash with unmerged paths to a worktree/index, possibly containing unmerged paths of its own, possibly getting new unmerged paths by failing to apply the stash, you get unmerged paths from several different unresolved conflicts. Git has no idea about the history of unmerged paths. So having git add modify the operation of git reset whenever git add overwrites an unmerged path in the index could lead to quite funny results. -- David Kastrup -- 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] builtin/pack-objects.c:write_pack_file() replace tmpname with pack_tmp_name in warning
Signed-off-by: Sun He sunheeh...@gmail.com --- The tmpname is uninitialized and it should a bug Please ignore the former patches about this with wrong format. I am sorry to cause a jam in your inbox. ^_^ In the end, I wanna thank Michael Haggerty who give me help. builtin/pack-objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c733379..4922ce5 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -823,7 +823,7 @@ static void write_pack_file(void) utb.modtime = --last_mtime; if (utime(pack_tmp_name, utb) 0) warning(failed utime() on %s: %s, - tmpname, strerror(errno)); + pack_tmp_name, strerror(errno)); } /* Enough space for -sha-1.pack? */ -- 1.9.0.138.g2de3478.dirty -- 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 v2 1/2] Add docstrings for lookup_replace_object() and do_lookup_replace_object()
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
cache.h | 13 +
replace_object.c | 7 +++
2 files changed, 20 insertions(+)
diff --git a/cache.h b/cache.h
index b039abc..9407560 100644
--- a/cache.h
+++ b/cache.h
@@ -798,13 +798,26 @@ static inline void *read_sha1_file(const unsigned char
*sha1, enum object_type *
{
return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
}
+
+/*
+ * This internal function is only declared here for the benefit of
+ * lookup_replace_object(). Please do not call it directly.
+ */
extern const unsigned char *do_lookup_replace_object(const unsigned char
*sha1);
+
+/*
+ * If object sha1 should be replaced, return the replacement object's
+ * name (replaced recursively, if necessary). The return value is
+ * either sha1 or a pointer to a permanently-allocated value. When
+ * object replacement is suppressed, always return sha1.
+ */
static inline const unsigned char *lookup_replace_object(const unsigned char
*sha1)
{
if (!check_replace_refs)
return sha1;
return do_lookup_replace_object(sha1);
}
+
static inline const unsigned char *lookup_replace_object_extended(const
unsigned char *sha1, unsigned flag)
{
if (!(flag LOOKUP_REPLACE_OBJECT))
diff --git a/replace_object.c b/replace_object.c
index c5cf9f4..31fabde 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -92,6 +92,13 @@ static void prepare_replace_object(void)
/* We allow recursive replacement. Only within reason, though */
#define MAXREPLACEDEPTH 5
+/*
+ * If a replacement for object sha1 has been set up, return the
+ * replacement object's name (replaced recursively, if necessary).
+ * The return value is either sha1 or a pointer to a
+ * permanently-allocated value. This function always respects replace
+ * references, regardless of the value of check_replace_refs.
+ */
const unsigned char *do_lookup_replace_object(const unsigned char *sha1)
{
int pos, depth = MAXREPLACEDEPTH;
--
1.8.5.3
--
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 v2 0/2] More object-related docstrings
This patch series applies on top of mh/replace-refs-variable-rename, simply because one of the comments refers to the global variable check_replace_refs by its new name. This is a re-roll of patches 1/6 and 6/6 of the series mh/object-code-cleanup that I submitted earlier [1]. Patches 2-5 of that series have already been queued. The first patch emphasizes that do_lookup_replace_object() is only meant for internal use, and moves the real docstring for that function from the header file to the implementation file. The second patch changes the docstring for hash_obj() to mention that its return value is not consistent across architectures, and adds a comment within the function explaining some points about the implementation that were suggested in the discussion about v1. Thanks to Junio, Christian, Nicolas, Jakub, and Jonathan for their comments on v1. Michael [1] http://thread.gmane.org/gmane.comp.version-control.git/242469 Michael Haggerty (2): Add docstrings for lookup_replace_object() and do_lookup_replace_object() Document some functions defined in object.c cache.h | 13 + object.c | 29 - object.h | 7 +++ replace_object.c | 7 +++ 4 files changed, 55 insertions(+), 1 deletion(-) -- 1.8.5.3 -- 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 v2 2/2] Document some functions defined in object.c
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
object.c | 29 -
object.h | 7 +++
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/object.c b/object.c
index 584f7ac..57a0890 100644
--- a/object.c
+++ b/object.c
@@ -43,14 +43,32 @@ int type_from_string(const char *str)
die(invalid object type \%s\, str);
}
+/*
+ * Return a numerical hash value between 0 and n-1 for the object with
+ * the specified sha1. n must be a power of 2. Please note that the
+ * return value is *not* consistent across computer architectures.
+ */
static unsigned int hash_obj(const unsigned char *sha1, unsigned int n)
{
unsigned int hash;
+
+ /*
+* Since the sha1 is essentially random, we just take the
+* required number of bits directly from the first
+* sizeof(unsigned int) bytes of sha1. First we have to copy
+* the bytes into a properly aligned integer. If we cared
+* about getting consistent results across architectures, we
+* would have to call ntohl() here, too.
+*/
memcpy(hash, sha1, sizeof(unsigned int));
- /* Assumes power-of-2 hash sizes in grow_object_hash */
return hash (n - 1);
}
+/*
+ * Insert obj into the hash table hash, which has length size (which
+ * must be a power of 2). On collisions, simply overflow to the next
+ * empty bucket.
+ */
static void insert_obj_hash(struct object *obj, struct object **hash, unsigned
int size)
{
unsigned int j = hash_obj(obj-sha1, size);
@@ -63,6 +81,10 @@ static void insert_obj_hash(struct object *obj, struct
object **hash, unsigned i
hash[j] = obj;
}
+/*
+ * Look up the record for the given sha1 in the hash map stored in
+ * obj_hash. Return NULL if it was not found.
+ */
struct object *lookup_object(const unsigned char *sha1)
{
unsigned int i, first;
@@ -92,6 +114,11 @@ struct object *lookup_object(const unsigned char *sha1)
return obj;
}
+/*
+ * Increase the size of the hash map stored in obj_hash to the next
+ * power of 2 (but at least 32). Copy the existing values to the new
+ * hash map.
+ */
static void grow_object_hash(void)
{
int i;
diff --git a/object.h b/object.h
index dc5df8c..732bf4d 100644
--- a/object.h
+++ b/object.h
@@ -42,7 +42,14 @@ struct object {
extern const char *typename(unsigned int type);
extern int type_from_string(const char *str);
+/*
+ * Return the current number of buckets in the object hashmap.
+ */
extern unsigned int get_max_object_index(void);
+
+/*
+ * Return the object from the specified bucket in the object hashmap.
+ */
extern struct object *get_indexed_object(unsigned int);
/*
--
1.8.5.3
--
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] finish_tmp_packfile():use strbuf for pathname construction
Signed-off-by: Sun He sunheeh...@gmail.com
---
I follow the suggestions of Eric Sunshine to fix the patch.
Of cause this patch has assumed that you have already fix the bug of
tmpname in builtin/pack-objects.c:write_pack_file() warning()
I want to say thank you to Eric Sunshine and Michael Haggerty who give me
lots of help.
builtin/pack-objects.c | 15 ++-
bulk-checkin.c | 8 +---
pack-write.c | 18 ++
pack.h | 2 +-
4 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..099d6ed 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -803,7 +803,7 @@ static void write_pack_file(void)
if (!pack_to_stdout) {
struct stat st;
- char tmpname[PATH_MAX];
+ struct strbuf tmpname = STRBUF_INIT;
/*
* Packs are runtime accessed in their mtime
@@ -826,23 +826,19 @@ static void write_pack_file(void)
tmpname, strerror(errno));
}
- /* Enough space for -sha-1.pack? */
- if (sizeof(tmpname) = strlen(base_name) + 50)
- die(pack base name '%s' too long, base_name);
- snprintf(tmpname, sizeof(tmpname), %s-, base_name);
+ strbuf_addf(tmpname, %s-, base_name);
if (write_bitmap_index) {
bitmap_writer_set_checksum(sha1);
bitmap_writer_build_type_index(written_list,
nr_written);
}
- finish_tmp_packfile(tmpname, pack_tmp_name,
+ finish_tmp_packfile(tmpname, pack_tmp_name,
written_list, nr_written,
pack_idx_opts, sha1);
if (write_bitmap_index) {
- char *end_of_name_prefix = strrchr(tmpname, 0);
- sprintf(end_of_name_prefix, %s.bitmap,
sha1_to_hex(sha1));
+ strbuf_addf(tmpname, %s.bitmap
,sha1_to_hex(sha1));
stop_progress(progress_state);
@@ -851,10 +847,11 @@ static void write_pack_file(void)
bitmap_writer_select_commits(indexed_commits,
indexed_commits_nr, -1);
bitmap_writer_build(to_pack);
bitmap_writer_finish(written_list, nr_written,
-tmpname,
write_bitmap_options);
+tmpname.buf,
write_bitmap_options);
write_bitmap_index = 0;
}
+ strbuf_release(tmpname);
free(pack_tmp_name);
puts(sha1_to_hex(sha1));
}
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..98e651c 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -4,6 +4,7 @@
#include bulk-checkin.h
#include csum-file.h
#include pack.h
+#include strbuf.h
static int pack_compression_level = Z_DEFAULT_COMPRESSION;
@@ -23,7 +24,7 @@ static struct bulk_checkin_state {
static void finish_bulk_checkin(struct bulk_checkin_state *state)
{
unsigned char sha1[20];
- char packname[PATH_MAX];
+ struct strbuf packname = STRBUF_INIT;
int i;
if (!state-f)
@@ -43,8 +44,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state
*state)
close(fd);
}
- sprintf(packname, %s/pack/pack-, get_object_directory());
- finish_tmp_packfile(packname, state-pack_tmp_name,
+ strbuf_addf(packname, %s/pack/pack-, get_object_directory());
+ finish_tmp_packfile(packname, state-pack_tmp_name,
state-written, state-nr_written,
state-pack_idx_opts, sha1);
for (i = 0; i state-nr_written; i++)
@@ -54,6 +55,7 @@ clear_exit:
free(state-written);
memset(state, 0, sizeof(*state));
+ strbuf_release(packname);
/* Make objects we just wrote available to ourselves */
reprepare_packed_git();
}
diff --git a/pack-write.c b/pack-write.c
index 9b8308b..9ccf804 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)
return sha1fd(fd, *pack_tmp_name);
}
-void finish_tmp_packfile(char *name_buffer,
+void finish_tmp_packfile(struct strbuf *name_buffer,
const char *pack_tmp_name,
struct pack_idx_entry **written_list,
uint32_t nr_written,
@@ -344,7 +344,7 @@
git reset path returns unwanted failure status
The use case: I'm doing a 'git stash pop'; it had conflicts. At this point, 'git status' shows: # On branch master # Changes to be committed: # (use git reset HEAD file... to unstage) # # modified: Target.java # # Unmerged paths: # (use git reset HEAD file... to unstage) # (use git add/rm file... as appropriate to mark resolution) # # both modified: DriveByInches.java # # Changes not staged for commit: # (use git add file... to update what will be committed) # (use git checkout -- file... to discard changes in working directory) # # modified: CommandBasedAutonomous.java # # Untracked files: # (use git add file... to include in what will be committed) # # AerialAssist2014/src/org/usfirst/frc1939/AerialAssist2014/Autonomous/ As part of the conflict resolution, I decide to unstage Target.java: stephe@takver$ git reset Target.java Unstaged changes after reset: M CommandBasedAutonomous.java U DriveByInches.java M Target.java stephe@takver$ echo $? 1 The issue is the error status and the messages about other files. If I had not specified a path to 'git reset', the error status would make sense; those files were not reset. However, since the file I specified was reset, there should be no error. Similarly, if I specify no path to a git command, I expect warning messages about files in the workspace that might need attention. However, if I do specify a path, I expect warning messages about files in that path only. This can be stated more concisely if the default path is considered to be * (and recursive); don't error if the operation succeeded for all files in the path; don't warn about files not in the path. -- -- 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: [PATCH 17/19] Portable alloca for Git
On Fri, Feb 28, 2014 at 02:50:04PM +0100, Erik Faye-Lund wrote: On Fri, Feb 28, 2014 at 2:44 PM, Erik Faye-Lund kusmab...@gmail.com wrote: On Mon, Feb 24, 2014 at 5:21 PM, Kirill Smelkov k...@mns.spb.ru wrote: diff --git a/Makefile b/Makefile index dddaf4f..0334806 100644 --- a/Makefile +++ b/Makefile @@ -316,6 +321,7 @@ endif ifeq ($(uname_S),Windows) GIT_VERSION := $(GIT_VERSION).MSVC pathsep = ; + HAVE_ALLOCA_H = YesPlease NO_PREAD = YesPlease NEEDS_CRYPTO_WITH_SSL = YesPlease NO_LIBGEN_H = YesPlease In MSVC, alloca is defined in in malloc.h, not alloca.h: http://msdn.microsoft.com/en-us/library/wb1s57t5.aspx In fact, it has no alloca.h at all. But we don't have malloca.h in mingw either, so creating a compat/win32/alloca.h that includes malloc.h is probably sufficient. But we don't have alloca.h in mingw either, sorry. Don't we have that for MSVC already in compat/vcbuild/include/alloca.h and ifeq ($(uname_S),Windows) ... BASIC_CFLAGS = ... -Icompat/vcbuild/include ... in config.mak.uname ? And as I've not touched MINGW part in config.mak.uname the patch stays valid as it is :) and we can incrementally update what platforms have working alloca with follow-up patches. In fact that would be maybe preferred, for maintainers to enable alloca with knowledge and testing, as one person can't have them all at hand. Thanks, Kirill -- 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] repack: add `repack.honorpackkeep` config var
On Feb 28, 2014, at 1:55 AM, Jeff King p...@peff.net wrote:
On Thu, Feb 27, 2014 at 10:04:44AM -0800, Junio C Hamano wrote:
I wonder if it makes sense to link it with pack.writebitmaps more
tightly, without even exposing it as a seemingly orthogonal knob
that can be tweaked, though.
I think that is because I do not fully understand the , because ...
part of the below:
This patch introduces an option to disable the
`--honor-pack-keep` option. It is not triggered by default,
even when pack.writeBitmaps is turned on, because its use
depends on your overall packing strategy and use of .keep
files.
If you ask --write-bitmap-index (or have pack.writeBitmaps on), you
do want the bitmap-index to be written, and unless you tell
pack-objects to ignore the .keep marker, it cannot do so, no?
Does the , because ... part above mean you may have an overall
packing strategy to use .keep file to not ever repack some subset of
the objects, so we will not silently explode the kept objects into a
new pack?
Exactly. The two features (bitmaps and .keep) are not compatible with
each other, so you have to prioritize one. If you are using static .keep
files, you might want them to continue being respected at the expense of
using bitmaps for that repo. So I think you want a separate option from
--write-bitmap-index to allow the appropriate flexibility.
Has anyone thought about how to make them compatible? We're using Martin Fick's
git-exproll script which makes heavy use of keeps to reduce pack file churn. In
addition to the on-disk benefits we get there, the driving factor behind
creating exproll was to prevent Gerrit from having two large (30GB+) mostly
duplicated pack files open in memory at the same time. Repacking in JGit would
help in a single-master environment, but we'd be back to having this problem
once we go to a multi-master setup.
Perhaps the solution here is actually something in JGit where it could
aggressively try to close references to pack files, but that still doesn't help
the disk churn problem. As Peff says below, we would want to repack often to
get up-to-date bitmaps, but ideally we could do that without writing hundreds
of GBs to disk (which is obviously worse when disk is a NFS mount).
The default is another matter. I think most people using .bitmaps on a
server would probably want to set repack.packKeptObjects. They would
want to repack often to take advantage of the .bitmaps anyway, so they
probably don't care about .keep files (any they see are due to races
with incoming pushes).
So we could do something like falling back to turning the option on if
--write-bitmap-index is on _and_ the user didn't specify
--pack-kept-objects. The existing default is mostly there because it is
the conservative choice (.keep files continue to do their thing as
normal unless you say otherwise). But the fallback thing would be one
less knob that bitmap users would need to turn in the common case.
Here's the interdiff for doing the fallback:
---
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3a3d84f..a8ddc7f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2139,7 +2139,9 @@ repack.usedeltabaseoffset::
repack.packKeptObjects::
If set to true, makes `git repack` act as if
`--pack-kept-objects` was passed. See linkgit:git-repack[1] for
- details. Defaults to false.
+ details. Defaults to `false` normally, but `true` if a bitmap
+ index is being written (either via `--write-bitmap-index` or
+ `pack.writeBitmaps`).
rerere.autoupdate::
When set to true, `git-rerere` updates the index with the
diff --git a/builtin/repack.c b/builtin/repack.c
index 49947b2..6b0b62d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -9,7 +9,7 @@
#include argv-array.h
static int delta_base_offset = 1;
-static int pack_kept_objects;
+static int pack_kept_objects = -1;
static char *packdir, *packtmp;
static const char *const git_repack_usage[] = {
@@ -190,6 +190,9 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
argc = parse_options(argc, argv, prefix, builtin_repack_options,
git_repack_usage, 0);
+ if (pack_kept_objects 0)
+ pack_kept_objects = write_bitmap;
+
packdir = mkpathdup(%s/pack, get_object_directory());
packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, (int)getpid());
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index f8431a8..b1eed5c 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -21,7 +21,7 @@ test_expect_success 'objects in packs marked .keep are not
repacked' '
objsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 |
sed -e s/^\([0-9a-f]\{40\}\).*/\1/)
mv pack-* .git/objects/pack/
- git repack -A -d -l
+ git repack --no-pack-kept-objects -A -d -l
git prune-packed
for p in
Re: [PATCH/RFC] rebase: new convenient option to edit a single commit
From: Jeff King p...@peff.net I'd expect -$n to mean rebase the last $n commits (as opposed to everything not in the upstream). That does not work currently, of course, but: 1. It has the potential to confuse people who read it, since it's unlike what -1 means in most of the rest of git. 2. It closes the door if we want to support -$n in the future. Yeah, rebase the last $n commits would be a nice touch. git rebase -i -10 --onto v1.9.0 # rebase the last 10 commits in this branch etc. Philip -- 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 17/19] Portable alloca for Git
On Fri, Feb 28, 2014 at 6:00 PM, Kirill Smelkov k...@mns.spb.ru wrote: On Fri, Feb 28, 2014 at 02:50:04PM +0100, Erik Faye-Lund wrote: On Fri, Feb 28, 2014 at 2:44 PM, Erik Faye-Lund kusmab...@gmail.com wrote: On Mon, Feb 24, 2014 at 5:21 PM, Kirill Smelkov k...@mns.spb.ru wrote: diff --git a/Makefile b/Makefile index dddaf4f..0334806 100644 --- a/Makefile +++ b/Makefile @@ -316,6 +321,7 @@ endif ifeq ($(uname_S),Windows) GIT_VERSION := $(GIT_VERSION).MSVC pathsep = ; + HAVE_ALLOCA_H = YesPlease NO_PREAD = YesPlease NEEDS_CRYPTO_WITH_SSL = YesPlease NO_LIBGEN_H = YesPlease In MSVC, alloca is defined in in malloc.h, not alloca.h: http://msdn.microsoft.com/en-us/library/wb1s57t5.aspx In fact, it has no alloca.h at all. But we don't have malloca.h in mingw either, so creating a compat/win32/alloca.h that includes malloc.h is probably sufficient. But we don't have alloca.h in mingw either, sorry. Don't we have that for MSVC already in compat/vcbuild/include/alloca.h and ifeq ($(uname_S),Windows) ... BASIC_CFLAGS = ... -Icompat/vcbuild/include ... in config.mak.uname ? Ah, of course. Thanks for setting me straight! And as I've not touched MINGW part in config.mak.uname the patch stays valid as it is :) and we can incrementally update what platforms have working alloca with follow-up patches. In fact that would be maybe preferred, for maintainers to enable alloca with knowledge and testing, as one person can't have them all at hand. Yeah, you're probably right. -- 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: [RFC 0/3] Make git more user-friendly during a merge conflict
David Kastrup d...@gnu.org writes: Stephen Leake stephen_le...@stephe-leake.org writes: David Kastrup d...@gnu.org writes: do the right thing commands also tend to do the wrong thing occasionally with potentially disastrous results when they are used in scripts where the followup actions rely on the actual result. That is bad, and should not be allowed. On the other hand, I have yet to see an actual use case of bad behavior in this discussion. Huh. http://permalink.gmane.org/gmane.comp.version-control.git/242744 That's about backward incompatibility, which is bad, but not what I was talking about above. Specifically, the proposed change is: 'git reset' will have different default actions depending on context: - if a merge is not in progress, it will do 'git reset --mixed' - if a merge is in progress, it will do 'git reset --merge' Is there a use case where this will do the wrong thing? Of course, I fully understand that not being able to come up with a wrong thing use case is not the same as proving it cannot happen, especially for a system as complex as git. So it would be ok to say we don't do that so we are not exposed to unintended consequences. But wrong thing use cases are more convincing :). -- -- 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: `git stash pop` UX Problem
Matthieu Moy matthieu@grenoble-inp.fr writes: Stephen Leake stephen_le...@stephe-leake.org writes: So it appears that adding a file _does_ tell git that the conflict is resolved. Yes it does. Git _knows_ that you consider the conflict to be resolved. It cannot know how happy you are with the result. Similarly, in a conflicted merge, the last git add does not trigger a commit silently. And a silent commit would be much less serious than a silent data drop. Ok, I see your point now. So a message merge complete; you can drop the stash would be the most git should do. -- -- 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: `git stash pop` UX Problem
David Kastrup d...@gnu.org writes: Stephen Leake stephen_le...@stephe-leake.org writes: Brandon McCaig bamcc...@gmail.com writes: On Thu, Feb 27, 2014 at 9:57 PM, Stephen Leake stephen_le...@stephe-leake.org wrote: You might be adding other files for other reasons. But if you add a file that does resolve a conflict caused by 'git stash pop', it is not guessing. Staging a file doesn't tell git that you resolved a conflict. Git will happily accept a blob full of conflict markers. Git doesn't know the difference. Git expects the user to know what is right. The user has the freedom to manipulate the index as they see fit, which means both adding and removing from it anytime they wish. But git has a notion of unresolved conflict. Not really. It has a notion of unmerged path. snip The index contains the unmerged versions of the file. Possibly also the version with conflict markers, but it's been too long since I last checked. Paraphrasing, is this correct? the index contains both versions of the unmerged file; any file with more than one version in the index is unmerged. So what 'git add' does in this case is replace both versions of the file in the index with a new version. I was not aware that the git system could support more than one version of a file in one branch. That makes it more like monotone :). If you apply a stash with unmerged paths to a worktree/index, possibly containing unmerged paths of its own, possibly getting new unmerged paths by failing to apply the stash, you get unmerged paths from several different unresolved conflicts. Yes; doing too many things at once is a bad idea. But that should never cause git to lose data or do something wrong. At the same time, it seems all unmerged paths result from unresolved merge conflicts, so the two notions are equivalent for git? Git has no idea about the history of unmerged paths. So having git add modify the operation of git reset whenever git add overwrites an unmerged path in the index could lead to quite funny results. Ok; I'll take that as describing a large class of bad thing use cases. -- -- 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: `git stash pop` UX Problem
Stephen Leake stephen_le...@stephe-leake.org writes: Matthieu Moy matthieu@grenoble-inp.fr writes: li...@haller-berlin.de (Stefan Haller) writes: Your intention was clearly to drop the stash, it just wasn't dropped because of the conflict. Dropping it automatically once the conflict is resolved would be nice. Your intention when you ran git stash pop, yes. Your intention when you ran git add, I call that guessing. You might be adding other files for other reasons. But if you add a file that does resolve a conflict caused by 'git stash pop', it is not guessing. The only thing you know for sure is that the user has consumed _one_ part of the stashed change, no? What if the stash had changes for more than one path? At the time of git add $path, can you reliably tell if the conflict to the $path the user is resolving came from a previous git stash pop, not from any other mergy operations, e.g. git stash apply or git apply -3? -- 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 2/2] Document some functions defined in object.c
On Fri, 28 Feb 2014, Michael Haggerty wrote:
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
Acked-by: Nicolas Pitre n...@fluxnic.net
---
object.c | 29 -
object.h | 7 +++
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/object.c b/object.c
index 584f7ac..57a0890 100644
--- a/object.c
+++ b/object.c
@@ -43,14 +43,32 @@ int type_from_string(const char *str)
die(invalid object type \%s\, str);
}
+/*
+ * Return a numerical hash value between 0 and n-1 for the object with
+ * the specified sha1. n must be a power of 2. Please note that the
+ * return value is *not* consistent across computer architectures.
+ */
static unsigned int hash_obj(const unsigned char *sha1, unsigned int n)
{
unsigned int hash;
+
+ /*
+ * Since the sha1 is essentially random, we just take the
+ * required number of bits directly from the first
+ * sizeof(unsigned int) bytes of sha1. First we have to copy
+ * the bytes into a properly aligned integer. If we cared
+ * about getting consistent results across architectures, we
+ * would have to call ntohl() here, too.
+ */
memcpy(hash, sha1, sizeof(unsigned int));
- /* Assumes power-of-2 hash sizes in grow_object_hash */
return hash (n - 1);
}
+/*
+ * Insert obj into the hash table hash, which has length size (which
+ * must be a power of 2). On collisions, simply overflow to the next
+ * empty bucket.
+ */
static void insert_obj_hash(struct object *obj, struct object **hash,
unsigned int size)
{
unsigned int j = hash_obj(obj-sha1, size);
@@ -63,6 +81,10 @@ static void insert_obj_hash(struct object *obj, struct
object **hash, unsigned i
hash[j] = obj;
}
+/*
+ * Look up the record for the given sha1 in the hash map stored in
+ * obj_hash. Return NULL if it was not found.
+ */
struct object *lookup_object(const unsigned char *sha1)
{
unsigned int i, first;
@@ -92,6 +114,11 @@ struct object *lookup_object(const unsigned char *sha1)
return obj;
}
+/*
+ * Increase the size of the hash map stored in obj_hash to the next
+ * power of 2 (but at least 32). Copy the existing values to the new
+ * hash map.
+ */
static void grow_object_hash(void)
{
int i;
diff --git a/object.h b/object.h
index dc5df8c..732bf4d 100644
--- a/object.h
+++ b/object.h
@@ -42,7 +42,14 @@ struct object {
extern const char *typename(unsigned int type);
extern int type_from_string(const char *str);
+/*
+ * Return the current number of buckets in the object hashmap.
+ */
extern unsigned int get_max_object_index(void);
+
+/*
+ * Return the object from the specified bucket in the object hashmap.
+ */
extern struct object *get_indexed_object(unsigned int);
/*
--
1.8.5.3
--
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
Re: [PATCH] archive: add archive.restrictRemote option
Jeff King p...@peff.net writes: On Thu, Feb 27, 2014 at 10:37:30AM -0800, Junio C Hamano wrote: Signed-off-by: Jeff King p...@peff.net Thanks. Do GitHub people have general aversion against signing off (or sending out, for that matter) their own patches, unless they were already active here before they joined GitHub, by the way? Mostly it is that I clean up the patches and commit messages before sending them out. Michael sends out his own patches because they are already perfect by the time I see them. :) I can certainly get S-O-B from GitHubbers, but my impression of the DCO is that it does not matter... ... A S-O-B from the author would perhaps make it more obvious what happened. Oh, I was not saying the practice was not legit. It was just that I expected a bit more from GitHub, a leading company that evangelises use of Git ;-) I was hoping to be vague. If we really want to get into specifics, we should probably document the current rules (refnames, and sub-trees of refnames). It might be a good thing to document that anyway, though. And by doing so, it would become obvious why one would want to set this option. I'll see what I can come up with. 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 v2 0/2] lifting upload-archive restrictions
Jeff King p...@peff.net writes: Here's a series to handle this; I think the end result is much nicer. I ended up calling the option uploadarchive.allowUnreachable. By moving it there instead of under archive, it is more clear that this is about the _serving_ end of the remote connection, and the word remote becomes redundant. Yeah, the original was already good but I like this version much better. I'm glad I bikeshedded ;-) 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: [RFC 0/3] Make git more user-friendly during a merge conflict
Stephen Leake stephen_le...@stephe-leake.org writes: David Kastrup d...@gnu.org writes: Stephen Leake stephen_le...@stephe-leake.org writes: David Kastrup d...@gnu.org writes: do the right thing commands also tend to do the wrong thing occasionally with potentially disastrous results when they are used in scripts where the followup actions rely on the actual result. That is bad, and should not be allowed. On the other hand, I have yet to see an actual use case of bad behavior in this discussion. Huh. http://permalink.gmane.org/gmane.comp.version-control.git/242744 That's about backward incompatibility, which is bad, but not what I was talking about above. No, it isn't. I quote: I sometimes run git reset during a merge to only reset the index and then examine the changes introduced by the merge. With your changes, someone doing so would abort the merge and discard the merge resolution. I very rarely do this, but even rarely, I wouldn't like Git to start droping data silently for me ;-). You should not make statements like I have yet to see an actual use case of bad behavior in this discussion when you actually mean I have not yet seen anything I would be interested in doing myself. -- David Kastrup -- 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 2/2] fetch: handle overlaping refspecs on --prune
Carlos Martín Nieto c...@elego.de writes: ... However, we now have 'origin/master' and 'origin/pr/5' both of which match the 'refs/remotes/origin/*' pattern. The current behaviour is to stop at the first match, which would mark it as stale as there is no 'refs/heads/pr/5' branch in the remote. OK, but with a later pattern, we can find out that it came from pull/5 that was advertised by the remote. If we had origin/pr/1 when the remote no longer has pull/1, then we can say that is stale. Makes sense. Thanks for an explanation. I wonder how well --prune would work on a repository in pre 1.5 layout, where all branches were copied to local refs/heads/ hierarchy except for 'master' (which is renamed to 'origin'). Does it have a similar issue? Do we end up pruning refs/heads/origin away because we do not see it on the remote end, or we somehow already deal with it and not have to worry about it? -- 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] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf
Hi,
Thanks for the suggestions and remarks.
I rewrote bulk-checkin.c:finish_bulk_checkin() using strbuf. But saw
that Sun He has already implemented the same way I have done.
Should I submit my implementation as a patch?
Secondly,
I tried implementing this WITHOUT changing the prototype of the
function pack-write.c:finish_tmp_packfile().
For this I detached the buffer from strbuf in finish_bulk_checkin()
using strbuf_detach() and passed it to finish_tmp_packfile().
Inside finish_tmp_packfile, I attached the same buffer to a local
struct strbuf using strbuf_attach().
Now the problem is, two of the arguments to strbuf_attach() are
'alloc' and 'len' which are private members of the struct strbuf.
But since I am just passing the detached buffer, the information of
alloc and len is lost which is required at the time of attaching.
I cannot think of any better way of using strbuf and NOT modify the
prototype of finish_tmp_packfile()
As a workaround, I can determine alloc = (strlen(buf) + 1) and len =
strlen(buf) but AFAIK this is not always true and may break.
Any suggestions?
Thanks.
On Fri, Feb 28, 2014 at 2:45 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Fri, Feb 28, 2014 at 2:58 AM, Faiz Kothari faiz.of...@gmail.com wrote:
Signed-off-by: Faiz Kothari faiz.of...@gmail.com
Notes:
I finally got what's happening, and why the errors were caused.
packname is supposed to contain the complete path to the .pack file.
Packs are stored as /path/to/SHA1.pack which I overlooked earlier.
After inspecting what is happening in pack-write.c:finish_tmp_packfile()
which indirectly modifies packname by appending the SHA1 and .pack to
packname
This is happening in these code snippets:
char *end_of_name_prefix = strrchr(name_buffer, 0);
and later
sprintf(end_of_name_prefix, %s.pack, sha1_to_hex(sha1));
name_buffer is packname.buf
Using const for the first argument of pack-write.c:finish_tmp_packfile()
doesnot raise any compile time warning or error and not any runtime
errors,
since the packname.buf is on heap and has extra space to which more char
can be written.
If this was not the case,
for e.g. passing a constant string and modifying it.
This will result in a segmentation fault.
---
This notes section is important to the ongoing email discussion,
however, it should be placed below the --- line so that it does not
become part of the recorded commit message when the patch is applied
via git am.
bulk-checkin.c |8 +---
pack-write.c |2 +-
pack.h |2 +-
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..bbdf1ec 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,7 @@ static struct bulk_checkin_state {
static void finish_bulk_checkin(struct bulk_checkin_state *state)
{
unsigned char sha1[20];
- char packname[PATH_MAX];
+ struct strbuf packname = STRBUF_INIT;
int i;
if (!state-f)
@@ -42,9 +42,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state
*state)
state-offset);
close(fd);
}
+ strbuf_addf(packname, %s/pack/pack-, get_object_directory());
+ strbuf_grow(packname, 40 + 5);
There are several problems with this. First, magic numbers 40 and 5
convey no meaning to the reader. At the very least, they should be
named constants or a comment should explain them. More seriously,
though, this code is fragile since it has far too intimate knowledge
of the inner workings of finish_tmp_packfile(). If the implementation
of finish_tmp_packfile() changes in the future such that it writes
more than 45 additional characters to the incoming buffer, this will
break.
Rather than coupling finish_bulk_checkin() and finish_tmp_packfile()
so tightly, consider finish_tmp_packfile() a black box which just
does its job and then propose ways to make things work without
finish_bulk_checkin() having to know how that job is done.
- sprintf(packname, %s/pack/pack-, get_object_directory());
- finish_tmp_packfile(packname, state-pack_tmp_name,
+ finish_tmp_packfile(packname.buf, state-pack_tmp_name,
state-written, state-nr_written,
state-pack_idx_opts, sha1);
for (i = 0; i state-nr_written; i++)
diff --git a/pack-write.c b/pack-write.c
index 605d01b..ac38867 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char
**pack_tmp_name)
return sha1fd(fd, *pack_tmp_name);
}
-void finish_tmp_packfile(char *name_buffer,
+void finish_tmp_packfile(const char *name_buffer,
This is misleading and fragile. By specifying 'const',
finish_tmp_packfile() promises not to modify the content of the
incoming name_buffer, yet it breaks this
Re: [PATCH] repack: add `repack.honorpackkeep` config var
Jeff King p...@peff.net writes: On Thu, Feb 27, 2014 at 10:04:44AM -0800, Junio C Hamano wrote: I wonder if it makes sense to link it with pack.writebitmaps more tightly, without even exposing it as a seemingly orthogonal knob that can be tweaked, though. I think that is because I do not fully understand the , because ... part of the below: This patch introduces an option to disable the `--honor-pack-keep` option. It is not triggered by default, even when pack.writeBitmaps is turned on, because its use depends on your overall packing strategy and use of .keep files. If you ask --write-bitmap-index (or have pack.writeBitmaps on), you do want the bitmap-index to be written, and unless you tell pack-objects to ignore the .keep marker, it cannot do so, no? Does the , because ... part above mean you may have an overall packing strategy to use .keep file to not ever repack some subset of the objects, so we will not silently explode the kept objects into a new pack? Exactly. The two features (bitmaps and .keep) are not compatible with each other, so you have to prioritize one. If you are using static .keep files, you might want them to continue being respected at the expense of using bitmaps for that repo. So I think you want a separate option from --write-bitmap-index to allow the appropriate flexibility. What is the appropriate flexibility, though? If the user wants to use bitmap, we would need to drop .keep, no? Doesn't always having two copies in two packs degrade performance unnecessarily (without even talking about wasted diskspace)? An explicit .keep exists in the repository because it is expensive and undesirable to duplicate what is in there in the first place, so it feels to me that either - Disable with warning, or outright refuse, the -b option if there is .keep (if we want to give precedence to .keep); or - Remove .keep with warning when -b option is given (if we want to give precedence to -b). and nothing else would be a reasonable option. Unfortunately, we can do neither automatically because there could be a transient .keep file in an active repository. So I think I agree with this... The default is another matter. I think most people using .bitmaps on a server would probably want to set repack.packKeptObjects. They would want to repack often to take advantage of the .bitmaps anyway, so they probably don't care about .keep files (any they see are due to races with incoming pushes). ... which makes me think that repack.packKeptObjects is merely a distraction---it should be enough to just pass --pack-kept-objects when -b is asked, without giving any extra configurability, no? So we could do something like falling back to turning the option on if --write-bitmap-index is on _and_ the user didn't specify --pack-kept-objects. If you mean didn't specify --no-pack-kept-objects, then I think that is sensible. I still do not know why we would want the configuration variable, 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: Branch Name Case Sensitivity
Karsten Blees karsten.bl...@gmail.com writes: If you are on a case-insensitive filesystem, or work on a cross-platform project, ensure that you avoid ambiguous refs. Problem solved. So its OK to lose data if you accidentally use an ambiguous ref? I cannot believe you actually meant that. I think he meant what he said: you avoid ambiguous refs. He did not say it is not Git's business to help you doing so. I think it is prudent to warn in the end-user facing layer (read: do not touch refs.c to implement something like that) when the user creates refs/heads/Next when there already is refs/heads/next, and I further think it would make sense to do so even on case sensitive platforms. We warn ambiguous refs across refs hierarchies (e.g. if you have refs/heads/next and refs/tags/next) with core.warnAmbiguousRefs; I do not think it is a stretch to either introduce a new configuration core.warnCaseInsensitiveRefs (auto-detected at the same place as we auto-detect core.ignorecase) or use the same core.warnAmbiguousRefs to trigger a warning upon seeing both refs/heads/next and refs/heads/Next. -- 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 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()
Michael Haggerty mhag...@alum.mit.edu writes: So my vote is that the patches are OK the way Dmitry wrote them (mind, I have only read through 05/11 so far). Seconded ;-) By the way, I do not like these long subjects. change is a redundant word when one sends a patch---as all patches are about changing something. Subject: builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path() would be a lot more appropriate for git shortlog consumption. -- 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 09/11] reflog-walk.c: use ALLOC_GROW() instead of inline code
Duy Nguyen pclo...@gmail.com writes: On Fri, Feb 28, 2014 at 4:46 PM, Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru wrote: Affected functions: read_one_reflog(), add_commit_info() We can usually see this from @@ line so it's not really needed to describe. Same comment for a few other patches. Not everybody always reads git log with -p. It is good to see what are changed mentioned somewhere. I prefer to see full sentences, though ;-) Subject: reflog-walk.c: use ALLOC_GROW() read_one_reflog() and add_commit_info() open-codes reallocation; use ALLOC_GROW() instead. or something. But that is minor. -- 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] help.c: rename function pretty_print_string_list
The part string_list of the name of function
pretty_print_string_list is just an implementation
detail. The function pretty-prints command names so
rename it to pretty_print_cmdnames.
Signed-off-by: Ralf Thielow ralf.thie...@gmail.com
---
Just noticed this while digging through Git codebase.
help.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/help.c b/help.c
index df7d16d..b266b09 100644
--- a/help.c
+++ b/help.c
@@ -78,8 +78,7 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames
*excludes)
cmds-cnt = cj;
}
-static void pretty_print_string_list(struct cmdnames *cmds,
-unsigned int colopts)
+static void pretty_print_cmdnames(struct cmdnames *cmds, unsigned int colopts)
{
struct string_list list = STRING_LIST_INIT_NODUP;
struct column_options copts;
@@ -209,14 +208,14 @@ void list_commands(unsigned int colopts,
const char *exec_path = git_exec_path();
printf_ln(_(available git commands in '%s'), exec_path);
putchar('\n');
- pretty_print_string_list(main_cmds, colopts);
+ pretty_print_cmdnames(main_cmds, colopts);
putchar('\n');
}
if (other_cmds-cnt) {
printf_ln(_(git commands available from elsewhere on your
$PATH));
putchar('\n');
- pretty_print_string_list(other_cmds, colopts);
+ pretty_print_cmdnames(other_cmds, colopts);
putchar('\n');
}
}
--
1.9.0.164.g473e143
--
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
t9200 cvsexportcommit test fails on Ubuntu server 12.04.4 LTS
Hi, I get 12 of 15 tests faling. Any idea? the same build works fine on 11.04 where I have a desktop version. Thanks not ok 1 - New file #mkdir A B C D E F # echo hello1 A/newfile1.txt # echo hello2 B/newfile2.txt # cp $TEST_DIRECTORY/test-binary- 1.png C/newfile3.png # cp $TEST_DIRECTORY/test-binary-1.png D/newfile4.png # git add A/newfile1.txt # git add B/newfile2.txt # git add C/newfile3.png # git add D/newfile4.png # git commit -a -m Test: New file # id=$(git rev-list --max-count=1 HEAD) # (cd $CVSWORK # git cvsexportcommit -c $id # check_entries A newfile1.txt/1.1/ # check_entries B newfile2.txt/1.1/ # check_entries C newfile3.png/1.1/-kb # check_entries D newfile4.png/1.1/-kb # test_cmp A/newfile1.txt ../A/newfile1.txt # test_cmp B/newfile2.txt ../B/newfile2.txt # test_cmp C/newfile3.png ../C/newfile3.png # test_cmp D/newfile4.png ../D/newfile4.png # ) not ok 2 - Remove two files, add two and update two #echo Hello1 A/newfile1.txt # rm -f B/newfile2.txt # rm -f C/newfile3.png # echo Hello5 E/newfile5.txt # cp $TEST_DIRECTORY/test-binary-2.png D/newfile4.png # cp $TEST_DIRECTORY/test-binary-1.png F/newfile6.png # git add E/newfile5.txt # git add F/newfile6.png # git commit -a -m Test: Remove, add and update # id=$(git rev-list --max-count=1 HEAD) # (cd $CVSWORK # git cvsexportcommit -c $id # check_entries A newfile1.txt/1.2/ # check_entries B # check_entries C # check_entries D newfile4.png/1.2/-kb # check_entries E newfile5.txt/1.1/ # check_entries F newfile6.png/1.1/-kb # test_cmp A/newfile1.txt ../A/newfile1.txt # test_cmp D/newfile4.png ../D/newfile4.png # test_cmp E/newfile5.txt ../E/newfile5.txt # test_cmp F/newfile6.png ../F/newfile6.png # ) ok 3 - Fail to change binary more than one generation old not ok 4 - Remove only binary files #git reset --hard HEAD^^ # rm -f D/newfile4.png # git commit -a -m test: remove only a binary file # id=$(git rev-list --max-count=1 HEAD) # (cd $CVSWORK # git cvsexportcommit -c $id # check_entries A newfile1.txt/1.2/ # check_entries B # check_entries C # check_entries D # check_entries E newfile5.txt/1.1/ # check_entries F newfile6.png/1.1/-kb # test_cmp A/newfile1.txt ../A/newfile1.txt # test_cmp E/newfile5.txt ../E/newfile5.txt # test_cmp F/newfile6.png ../F/newfile6.png # ) not ok 5 - Remove only a text file #rm -f A/newfile1.txt # git commit -a -m test: remove only a binary file # id=$(git rev-list --max-count=1 HEAD) # (cd $CVSWORK # git cvsexportcommit -c $id # check_entries A # check_entries B # check_entries C # check_entries D # check_entries E newfile5.txt/1.1/ # check_entries F newfile6.png/1.1/-kb # test_cmp E/newfile5.txt ../E/newfile5.txt # test_cmp F/newfile6.png ../F/newfile6.png # ) not ok 6 - New file with spaces in file name #mkdir G g # echo ok then G g/with spaces.txt # git add G g/with spaces.txt \ # cp $TEST_DIRECTORY/test-binary-1.png G g/with spaces.png \ # git add G g/with spaces.png # git commit -a -m With spaces # id=$(git rev-list --max-count=1 HEAD) # (cd $CVSWORK # git cvsexportcommit -c $id # check_entries G g with spaces.png/1.1/-kb|with spaces.txt/1.1/ # ) not ok 7 - Update file with spaces in file name #echo Ok then G g/with spaces.txt # cat $TEST_DIRECTORY/test-binary-1.png G g/with spaces.png \ # git add G g/with spaces.png # git commit -a -m Update with spaces # id=$(git rev-list --max-count=1 HEAD) # (cd $CVSWORK # git cvsexportcommit -c $id # check_entries G g with spaces.png/1.2/-kb|with spaces.txt/1.2/ # ) not ok 8 - File with non-ascii file name #mkdir -p Å/goo/a/b/c/d/e/f/g/h/i/j/k/l/m/n/o/p/q/r/s/t/u/v/w/x/y/z/å/ä/ö # echo Foo Å/goo/a/b/c/d/e/f/g/h/i/j/k/l/m/n/o/p/q/r/s/t/u/v/w/x/y/z/å/ä/ö/gårdetsågårdet.txt # git add Å/goo/a/b/c/d/e/f/g/h/i/j/k/l/m/n/o/p/q/r/s/t/u/v/w/x/y/z/å/ä/ö/gårdetsågårdet.txt # cp $TEST_DIRECTORY/test-binary-1.png Å/goo/a/b/c/d/e/f/g/h/i/j/k/l/m/n/o/p/q/r/s/t/u/v/w/x/y/z/å/ä/ö/gårdetsågårdet.png # git add Å/goo/a/b/c/d/e/f/g/h/i/j/k/l/m/n/o/p/q/r/s/t/u/v/w/x/y/z/å/ä/ö/gårdetsågårdet.png # git commit -a -m Går det så går det \ # id=$(git rev-list --max-count=1 HEAD) # (cd
Re: `git stash pop` UX Problem
Stephen Leake stephen_le...@stephe-leake.org writes: I was not aware that the git system could support more than one version of a file in one branch. The index only. The history itself does not. -- 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] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE
Michael Haggerty mhag...@alum.mit.edu writes: On 02/28/2014 10:07 AM, Sun He wrote: Signed-off-by: Sun He sunheeh...@gmail.com --- parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index 7b8d3fa..59a52b0 100644 --- a/parse-options.c +++ b/parse-options.c @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts) case OPTION_NEGBIT: case OPTION_SET_INT: case OPTION_SET_PTR: -case OPTION_NUMBER: +case OPTION_CMDMODE: if ((opts-flags PARSE_OPT_OPTARG) || !(opts-flags PARSE_OPT_NOARG)) err |= optbug(opts, should not accept an argument); -- 1.9.0.138.g2de3478.dirty Hi, When I was reading code yesterday, I came across this protential bug. parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option. Please, no overlong line in the text part that makes things unnecessarily hard to read. I agree with all the comments in the message I am responding to. BTW, none of my comments should be taken to indicate whether the commit itself makes sense or not. I haven't checked that far. While I think it is sensible to make sure CMDMODE is not marked to allow arguments, I do not understand why special type would imply it is allowed to be marked to take an argument. Why is it a good change to remove case OPTION_NUMBER: 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: `git stash pop` UX Problem
Stephen Leake stephen_le...@stephe-leake.org writes: So a message merge complete; you can drop the stash would be the most git should do. From the user experience point of view, that would be good. It could bother some users, but we have advice.* to silent this kind of warnings. From the implementation point of view, it's much harder than it seems because as other pointed out, Git does not know that the merge conflicts comes from, so as it is, the best it could say is merge complete; you can now proceed. Thas is a solvable problem (git stash could leave a file like .git/conflicted-stash, and git add could look for this file and remove it), but I can't think of an implementation that would not be really awful. For example, git reset should also remove the file, and in general a substancial subset of Git's command would need to be aware of the status of git stash. So, I wouldn't object, but I don't think the implementation cost is worth the benefit. -- 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
