Git development
 help / color / mirror / Atom feed
* Re: Effectively tracing project contributions with git
From: Theodore Tso @ 2009-09-13  2:28 UTC (permalink / raw)
  To: Joseph Wakeling; +Cc: Sverre Rabbelier, Jeff King, git
In-Reply-To: <4AAC3889.6030908@webdrake.net>

On Sun, Sep 13, 2009 at 02:10:49AM +0200, Joseph Wakeling wrote:
> 
> I don't see any solution that doesn't see me browsing diffs -- there's
> no metric that will solve the problem -- but if your stats work could
> help me get an output of the form 'here are all the diffs on file X by
> contributor Y in order of size, largest first' then I think it would
> help a LOT.

This will display all of the diffs on file (pathname) XXX by contributor YYY:

	git log -p --author=YYY XXX 

You might also find the diffstats useful:

	git log --stat --author=YYY XXX

Or if you want *only* the diffstats for the file in question, you might try:

	git log --stat --pretty=format: --author=YYY XXX | grep XXX

So the bottom line is git will allow you to extract quite a lot of
information.  You might need to do some perl- or shell- or python-
scripting to analyze or format the information, but the harder
question is determining exactly what question you want to ask.

Eliminating whitespace changes isn't hard (add the -b flag).  If you
want to eliminate variable renaming, that's harder since that requires
actually parsing the patch.  There are programs that will do that
(normally used by University professors to catch students cheating at
Programming 101 courses :-), but you'd need to do some shell (or perl
or python) scripting to splice them into the git invocations to
extract out the information.

Is there a particular reason why this is important to you?  Is it for
curiosity reasons; are you trying to build a case that you've
contacted all of the significant contributors for the purposes of
changing the license used on a file?  If it's the latter, what I'd
probably do is just simply collect everyone who has ever changed a
file (git log --format="%aN <%aE>" pathname/to/a/file | sort -u) and
try to get as many people as possible to agree to the license change.
For the ones who have _not_ agreed, or which you can not contact, you
can go back and just analyze their changes (git log --author=YYY) to
decide whether or not they are significant, and whether you need to
try extract hard to contact them, or in the worst case, find someone
to rewrite the parts of the file which they had modified in the past.

Or maybe you have some other reason for gathering said information.
Depending on what the high-level thing it is that you are trying to
do, there may be an easier or more elegant way to get the information
you are requesting.

						- Ted

^ permalink raw reply

* [PATCH] quiltimport documentation: --dry-run and -n are synonyms
From: Junio C Hamano @ 2009-09-13  2:41 UTC (permalink / raw)
  To: git

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-quiltimport.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-quiltimport.txt b/Documentation/git-quiltimport.txt
index d4037de..579e8d2 100644
--- a/Documentation/git-quiltimport.txt
+++ b/Documentation/git-quiltimport.txt
@@ -9,7 +9,7 @@ git-quiltimport - Applies a quilt patchset onto the current branch
 SYNOPSIS
 --------
 [verse]
-'git quiltimport' [--dry-run] [--author <author>] [--patches <dir>]
+'git quiltimport' [--dry-run | -n] [--author <author>] [--patches <dir>]
 
 
 DESCRIPTION
-- 
1.6.5.rc0.82.g1c5d9

^ permalink raw reply related

* Re: [PATCH] git-push: Accept -n as a synonym for --dry-run.
From: Junio C Hamano @ 2009-09-13  2:44 UTC (permalink / raw)
  To: Nelson Elhage; +Cc: git
In-Reply-To: <1252800302-26560-1-git-send-email-nelhage@mit.edu>

Nelson Elhage <nelhage@MIT.EDU> writes:

> '-n' is the standard way to specify a dry run for other git commands,
> so make 'git-push' accept it as well.
> ---

Sign-off?

Indeed -n is used in many places for --dry-run, but it is not _the_
standard way.

commit, push (as you identified), reflog, and send-email have --dry-run
but -n is not a synonym for it.  Some of them even use -n as a shorthand
for a more often used option than --dry-run.

So the justification should be more like "push does not any other option
that deserves a short-and-sweet -n better, it will not have any such
option in the future, and --dry-run is very often used that it deserves to
use -n as its short-hand."

I tend to agree with the first two points, but I am not sure about the
third point.  Do people dry-push that often?

^ permalink raw reply

* Re: obnoxious CLI complaints
From: Junio C Hamano @ 2009-09-13  2:47 UTC (permalink / raw)
  To: John Tapsell; +Cc: Dmitry Potapov, Brendan Miller, Jakub Narebski, git
In-Reply-To: <43d8ce650909121608t2b9c4b9bw44104acceea26e12@mail.gmail.com>

John Tapsell <johnflux@gmail.com> writes:

> Ah, the manpage examples specifically give the --format=tar though.

So what?

> Why not have  --format=tgz  then or something?  Or better yet, give
> the filename on the command line and detect the format from the file
> extension.

That is an interesting enhancement and sounds like a useful feature.

^ permalink raw reply

* Re: [PATCH] preserve mtime of local clone
From: Junio C Hamano @ 2009-09-13  3:06 UTC (permalink / raw)
  To: Clemens Buchacher; +Cc: git, msysgit, Shawn O. Pearce
In-Reply-To: <20090912090348.GB9654@localhost>


Clemens Buchacher <drizzd@aon.at> writes:

> A local clone without hardlinks copies all objects, including dangling
> ones, to the new repository. Since the mtimes are renewed, those
> dangling objects cannot be pruned by "git gc --prune", even if they
> would have been old enough for pruning in the original repository.
>
> Instead, preserve mtime during copy. "git gc --prune" will then work
> in the clone just like it did in the original.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
>
> On Sat, Sep 12, 2009 at 10:26:24AM +0200, Clemens Buchacher wrote:
>
>> If it's a problem we can use utime() instead. I was just trying to use the
>> file descriptors, since they were available. But the patch would be a little
>> smaller if I didn't touch copy_fd().
>
> Here we go.

Thanks.

This new feature is not wanted by most of the callers of copy_file(), and
it is not like they may want to conditionally pass 1 in preserve_times
someday.  I'd limit the damage like this replacement patch, instead.

Is it unreasonable to have a test for this one, by the way?

 builtin-clone.c |    2 +-
 cache.h         |    1 +
 copy.c          |   21 +++++++++++++++++++++
 3 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index ad04808..bab2d84 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -269,7 +269,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest)
 				die_errno("failed to create link '%s'", dest->buf);
 			option_no_hardlinks = 1;
 		}
-		if (copy_file(dest->buf, src->buf, 0666))
+		if (copy_file_with_time(dest->buf, src->buf, 0666))
 			die_errno("failed to copy file to '%s'", dest->buf);
 	}
 	closedir(dir);
diff --git a/cache.h b/cache.h
index 867918d..1668f28 100644
--- a/cache.h
+++ b/cache.h
@@ -923,6 +923,7 @@ extern const char *git_mailmap_file;
 extern void maybe_flush_or_die(FILE *, const char *);
 extern int copy_fd(int ifd, int ofd);
 extern int copy_file(const char *dst, const char *src, int mode);
+extern int copy_file_with_time(const char *dst, const char *src, int mode);
 extern void write_or_die(int fd, const void *buf, size_t count);
 extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);
 extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);
diff --git a/copy.c b/copy.c
index e54d15a..a7f58fd 100644
--- a/copy.c
+++ b/copy.c
@@ -35,6 +35,19 @@ int copy_fd(int ifd, int ofd)
 	return 0;
 }
 
+static int copy_times(const char *dst, const char *src)
+{
+	struct stat st;
+	struct utimbuf times;
+	if (stat(src, &st) < 0)
+		return -1;
+	times.actime = st.st_atime;
+	times.modtime = st.st_mtime;
+	if (utime(dst, &times) < 0)
+		return -1;
+	return 0;
+}
+
 int copy_file(const char *dst, const char *src, int mode)
 {
 	int fdi, fdo, status;
@@ -55,3 +68,11 @@ int copy_file(const char *dst, const char *src, int mode)
 
 	return status;
 }
+
+int copy_file_with_time(const char *dst, const char *src, int mode)
+{
+	int status = copy_file(dst, src, mode);
+	if (!status)
+		return copy_times(dst, src);
+	return status;
+}

^ permalink raw reply related

* Re: [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot
From: Junio C Hamano @ 2009-09-13  3:30 UTC (permalink / raw)
  To: Mark Rada; +Cc: git, Jakub Narebski
In-Reply-To: <4AAC3833.8060905@mailservices.uwaterloo.ca>

Mark Rada <marada@uwaterloo.ca> writes:

> Makes things nicer in cases when you hand craft the snapshot URL but
> make a typo in defining the hash variable (e.g. netx instead of next);
> you will now get an error message instead of a broken tarball.
>
> To maintain backwards compatibility, git_get_head_hash is now a wrapper
> for git_get_full_hash, as suggested by Jakub Narebski.
>
> Tests for t9501 are included to demonstrate added functionality.
>
> Signed-off-by: Mark Rada <marada@uwaterloo.ca>
> ---
>
> 	This is just a re-send based on getting torn a new one by Junio.
> 	Changes since v3:
> 		- variables have been renamed for readability

Much nicer to read.  Thanks.

> @@ -5196,8 +5202,9 @@ sub git_snapshot {
>  		die_error(403, "Unsupported snapshot format");
>  	}
>  
> -	if (!defined $hash) {
> -		$hash = git_get_head_hash($project);
> +	my $full_hash = git_get_full_hash($project, $hash);
> +	if (!$full_hash) {
> +		die_error(404, 'Hash id was not valid');
>  	}

This is in the context of "snapshot", so obviously you care more about
just "such an object exists", don't you?  You also want it to be a
tree-ish.  Try giving it $hash = 'junio-gpg-pub' and see how it breaks.

> @@ -5210,7 +5217,7 @@ sub git_snapshot {
>  	$cmd = quote_command(
>  		git_cmd(), 'archive',
>  		"--format=$known_snapshot_formats{$format}{'format'}",
> -		"--prefix=$name/", $hash);
> +		"--prefix=$name/", $full_hash);

Why?  There was no justification as to why this change is necessary in the
commit log message.

^ permalink raw reply

* Re: [RFC/PATCH v4 2/2] gitweb: append short hash ids to snapshot files
From: Junio C Hamano @ 2009-09-13  3:35 UTC (permalink / raw)
  To: Mark Rada; +Cc: git, Junio C Hamano, Jakub Narebski
In-Reply-To: <4AAC2917.6000306@mailservices.uwaterloo.ca>

Mark Rada <marada@uwaterloo.ca> writes:

> Teach gitweb how to produce nicer snapshot names by only using the
> short hash id. If clients make requests using a tree-ish that is not a
> partial or full SHA-1 hash, then the short hash will also be appended
> to whatever they asked for.
> ...
> +sub git_get_short_hash {
> +	my $project = shift;
> +	my $hash = shift;
> +	my $o_git_dir = $git_dir;
> +	my $retval = undef;
> +	$git_dir = "$projectroot/$project";
> +	if (open my $fd, '-|', git_cmd(), 'rev-parse', '--short', $hash) {
> +		$hash = <$fd>;
> +		close $fd;
> +		if (defined $hash && $hash =~ /^([0-9a-fA-F]{7,})$/) {

If you want to make sure it is 7 or longer, ask rev-parse to give you 7 or
longer explicitly, so that you won't be hit by default changing under you
in the future.

> @@ -5207,6 +5227,12 @@ sub git_snapshot {
> ...
> +
> +	if ($full_hash !~ /$hash/) {
> +		$hash .= '-' . git_get_short_hash($project, $hash);
> +	} else {
> +		$hash = git_get_short_hash($project, $hash);
> +	}

I do not get this test.  What is this unanchored pattern match about?

I do not think you wanted to allow matching a partial 1234567 $hash to
substitute a full 01234567..... $full_hash, so I am guessing that you
meant to say "$full_hash !~ /^$hash/" at least, or perhaps you meant even
"$full_hash ne $hash".

But that still does not make much sense to me.  Perhaps you meant to catch
a case where $hash is a tagname (or refname), i.e. $hash = 'v1.6.3' or
$hash = 'next'?

If that is indeed the case, then perhaps you should check for that more
explicitly, perhaps using "git show-ref $hash" or something.  I do not
know if the complexity (not just the "detect handcrafted $hash string that
is not an SHA-1", but this whole "give shorten one" topic) is worth it,
though.  And if you drop the hunk that changes user supplied $hash to
$full_hash in the output file name in your [PATCH 1/2], I do not think you
need this anyway.  If somebody asked for 'next', he will get 'next'.

If somebody asked for 01234... (full 40 hexdigits) because that was the
link on the gitweb output page, it might make sense to give him a
shortened name, but then the above conditional needs to be only:

	if ($full_hash eq $hash) {
        	$hash = git_get_short_hash($project, $hash);
	}

no?        

> +test_commit \
> +	'SnapshotFileTests' \
> +	'i can has snapshot?'
> +test_expect_success \
> +	'snapshots: give full hash' \
> +	'ID=`git rev-parse --verify HEAD` &&
> +	gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
> +	ID=`git rev-parse --short HEAD` &&
> +	grep ".git-$ID.tar.gz" gitweb.output'

I'd rather see these indented like:

        test_expect_success 'snapshots: give full hash' '
		ID=$(git rev-parse --verify HEAD) &&
		gitweb_run ...
        '

Also, if I am not mistaken, "test_commit" is not about doing any test, but
is a short-hand for doing an operation, right?  It would be better to have
it inside test_expect_success just in case your "git commit" or some other
commands are broken.  I.e. like

	test_expect_success 'create a test commit' '
		test_commit SnapshotFileTests "Can I have shapshot?"
        '

^ permalink raw reply

* Re: [PATCH] git-push: Accept -n as a synonym for --dry-run.
From: Nelson Elhage @ 2009-09-13  3:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vfxar5zsi.fsf@alter.siamese.dyndns.org>

On Sat, Sep 12, 2009 at 07:44:29PM -0700, Junio C Hamano wrote:
> Sign-off?

Signed-off-by: Nelson Elhage <nelhage@mit.edu>

(I can resend the entire patch, but I'll resend with a new commit
message if appropriate after any discussion plays itself out).

> 
> Indeed -n is used in many places for --dry-run, but it is not _the_
> standard way.
> 
> commit, push (as you identified), reflog, and send-email have --dry-run
> but -n is not a synonym for it.  Some of them even use -n as a shorthand
> for a more often used option than --dry-run.

Can you point to an example of a git command supporting --dry-run, and
using -n for something else? I personally would find that confusing,
since -n is a common alias for dry-run both inside and outside of git
(c.f. make, rsync, libtool). I guess patch(1) has that property, but
none of your examples from git use -n to mean something else.

In fact, reflog already supports '-n' to mean dry-run, it's just not
documented. I'll send along a documentation patch to fix that.

I got the claim that -n was "standard" from parse-options.h, which
defines OPT__DRY_RUN, which defines both -n and --dry-run switches at
the same time. Given the number of commands that treat them as
synonymous, I think it would be a win for UI consistency to make them
synonymous everywhere.

> 
> So the justification should be more like "push does not any other option
> that deserves a short-and-sweet -n better, it will not have any such
> option in the future, and --dry-run is very often used that it deserves to
> use -n as its short-hand."
> 
> I tend to agree with the first two points, but I am not sure about the
> third point.  Do people dry-push that often?

I personally use --dry-run almost every time I push, which is what
inspired this patch. Especially now that the push.default can change
the behavior of push from repo to repo, I want to be sure I know what
I'm about to push. The recent 'git push --confirm' thread suggests I
am not the only person with this concern.

^ permalink raw reply

* [PATCH] reflog documentation: -n is an alias for --dry-run
From: Nelson Elhage @ 2009-09-13  3:41 UTC (permalink / raw)
  To: git; +Cc: Nelson Elhage

Signed-off-by: Nelson Elhage <nelhage@mit.edu>
---
 Documentation/git-reflog.txt |    2 +-
 builtin-reflog.c             |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
index 7f7a544..b434ff7 100644
--- a/Documentation/git-reflog.txt
+++ b/Documentation/git-reflog.txt
@@ -16,7 +16,7 @@ The command takes various subcommands, and different options
 depending on the subcommand:
 
 [verse]
-'git reflog expire' [--dry-run] [--stale-fix] [--verbose]
+'git reflog expire' [-n | --dry-run] [--stale-fix] [--verbose]
 	[--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...
 +
 'git reflog delete' ref@\{specifier\}...
diff --git a/builtin-reflog.c b/builtin-reflog.c
index 95198c5..f37c57b 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c
@@ -13,9 +13,9 @@
  */
 
 static const char reflog_expire_usage[] =
-"git reflog (show|expire) [--verbose] [--dry-run] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...";
+"git reflog (show|expire) [--verbose] [-n | --dry-run] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...";
 static const char reflog_delete_usage[] =
-"git reflog delete [--verbose] [--dry-run] [--rewrite] [--updateref] <refs>...";
+"git reflog delete [--verbose] [-n | --dry-run] [--rewrite] [--updateref] <refs>...";
 
 static unsigned long default_reflog_expire;
 static unsigned long default_reflog_expire_unreachable;
-- 
1.6.3.1.499.ge7b8da

^ permalink raw reply related

* Re: [PATCH] git-push: Accept -n as a synonym for --dry-run.
From: Nelson Elhage @ 2009-09-13  3:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <20090913034031.GO4275@mit.edu>

On Sat, Sep 12, 2009 at 11:40:31PM -0400, Nelson Elhage wrote:
> On Sat, Sep 12, 2009 at 07:44:29PM -0700, Junio C Hamano wrote:
> Can you point to an example of a git command supporting --dry-run, and
> using -n for something else? I personally would find that confusing,
> since -n is a common alias for dry-run both inside and outside of git
> (c.f. make, rsync, libtool). I guess patch(1) has that property, but
> none of your examples from git use -n to mean something else.

Oops, I missed 'commit' in your list, which I see (as of recently) has
that property. In that case, though, --dry-run was added after an
established -n option had been there for a long time; I think my
general argument still stands for commands where that is not the case.

- Nelson

^ permalink raw reply

* Re: [PATCH] git-push: Accept -n as a synonym for --dry-run.
From: Junio C Hamano @ 2009-09-13  5:18 UTC (permalink / raw)
  To: Nelson Elhage; +Cc: git
In-Reply-To: <20090913034031.GO4275@mit.edu>

Nelson Elhage <nelhage@MIT.EDU> writes:

>> commit, push (as you identified), reflog, and send-email have --dry-run
>> but -n is not a synonym for it.  Some of them even use -n as a shorthand
>> for a more often used option than --dry-run.
>
> Can you point to an example of a git command supporting --dry-run, and
> using -n for something else?

Doesn't commit -n stand for no-verify?  I think that is how it is
documented.

> In fact, reflog already supports '-n' to mean dry-run,

Huh?  "git reflog -n 4" is for show-four-entries-from-the-top.

^ permalink raw reply

* Re: [PATCH] git-push: Accept -n as a synonym for --dry-run.
From: Junio C Hamano @ 2009-09-13  5:23 UTC (permalink / raw)
  To: Nelson Elhage; +Cc: Junio C Hamano, git
In-Reply-To: <20090913035421.GP4275@mit.edu>

Nelson Elhage <nelhage@MIT.EDU> writes:

> ... I think my
> general argument still stands for commands where that is not the case.

Cool down.

It is a mere subset of what I already said, so you are not arguing against
me at all.

    So the justification should be more like "push does not have any other
    option that deserves a short-and-sweet -n better, it will not have any
    such option in the future, and --dry-run is very often used that it
    deserves to use -n as its short-hand."

and I already said I tend to agree with the first two points.  Indeed the
first point is an absolute truth (the statement is about the current
state).

To answer the second point you need to look into the future, but I do not
foresee us adding a very useful option to the command whose usefulness far
outweigh that of dry-run and whose name begins with 'n' to want to use it
as the short-hand.  In such a case, it is likely that we would try very
hard to find a name that does not begin with 'n' to avoid the issue.

^ permalink raw reply

* Re: [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot
From: Mark Rada @ 2009-09-13  5:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jakub Narebski
In-Reply-To: <7vmy4z4j35.fsf@alter.siamese.dyndns.org>

On 09-09-12 11:30 PM, Junio C Hamano wrote:
>> @@ -5196,8 +5202,9 @@ sub git_snapshot {
>>  		die_error(403, "Unsupported snapshot format");
>>  	}
>>  
>> -	if (!defined $hash) {
>> -		$hash = git_get_head_hash($project);
>> +	my $full_hash = git_get_full_hash($project, $hash);
>> +	if (!$full_hash) {
>> +		die_error(404, 'Hash id was not valid');
>>  	}
> 
> This is in the context of "snapshot", so obviously you care more about
> just "such an object exists", don't you?  You also want it to be a
> tree-ish.  Try giving it $hash = 'junio-gpg-pub' and see how it breaks.
 
You have confused me. How is using 'junio-gpg-pub' different from the 
second test case that tries to use 'frizzumFrazzum'?


-- 
Mark Rada (ferrous26)
marada@uwaterloo.ca

^ permalink raw reply

* Re: [RFC/PATCH v4 2/2] gitweb: append short hash ids to snapshot files
From: Mark Rada @ 2009-09-13  5:39 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Mark Rada, git, Jakub Narebski
In-Reply-To: <7v7hw34ivl.fsf@alter.siamese.dyndns.org>

On 09-09-12 11:35 PM, Junio C Hamano wrote:
>> @@ -5207,6 +5227,12 @@ sub git_snapshot {
>> ...
>> +
>> +	if ($full_hash !~ /$hash/) {
>> +		$hash .= '-' . git_get_short_hash($project, $hash);
>> +	} else {
>> +		$hash = git_get_short_hash($project, $hash);
>> +	}
> 
> I do not get this test.  What is this unanchored pattern match about?

I missed that, it should have been anchored.

> I do not think you wanted to allow matching a partial 1234567 $hash to
> substitute a full 01234567..... $full_hash, so I am guessing that you
> meant to say "$full_hash !~ /^$hash/" at least, or perhaps you meant even
> "$full_hash ne $hash".
> 
> But that still does not make much sense to me.  Perhaps you meant to catch
> a case where $hash is a tagname (or refname), i.e. $hash = 'v1.6.3' or
> $hash = 'next'?
> 
> If that is indeed the case, then perhaps you should check for that more
> explicitly, perhaps using "git show-ref $hash" or something.  I do not
> know if the complexity (not just the "detect handcrafted $hash string that
> is not an SHA-1", but this whole "give shorten one" topic) is worth it,
> though.  And if you drop the hunk that changes user supplied $hash to
> $full_hash in the output file name in your [PATCH 1/2], I do not think you
> need this anyway.  If somebody asked for 'next', he will get 'next'.
> 
> If somebody asked for 01234... (full 40 hexdigits) because that was the
> link on the gitweb output page, it might make sense to give him a
> shortened name, but then the above conditional needs to be only:
> 
> 	if ($full_hash eq $hash) {
>         	$hash = git_get_short_hash($project, $hash);
> 	}
> 
> no?

This was a manifestation of a suggestion from Jakub:
> Second, I'd rather have better names for snapshots than using full SHA-1.
> For snapshot of 'v1.5.0' of repository 'repo.git' I'd prefer for snapshot
> to be named 'repo-v1.5.0', and for snapshot of 'next' branch of the same
> project to be named for example 'repo-next-20090909', or perhaps
> 'repo-next-2009-09-10T09:16:18' or 'repo-next-20090909-g5f6b0ff',
> or 'repo-v1.6.5-rc0-164-g5f6b0ff'.
> 
> I'm not sure what would be the best name of snapshot of given 
> subdirectory...
> 
> 
> In short: I'd rather not improve on bad design of using full SHA-1
> in snapshot name.

For me, there are two fates that snapshots will end up with: being deleted
as soon as I have unrolled the contents, or long term archiving. For the
latter case, it is nice to have an idea of when it came from, though I
guess I should have appended a date in that case... ¯\(°_o)/¯

Thoughts?


>> +test_commit \
>> +	'SnapshotFileTests' \
>> +	'i can has snapshot?'
>> +test_expect_success \
>> +	'snapshots: give full hash' \
>> +	'ID=`git rev-parse --verify HEAD` &&
>> +	gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
>> +	ID=`git rev-parse --short HEAD` &&
>> +	grep ".git-$ID.tar.gz" gitweb.output'
> 
> I'd rather see these indented like:
> 
>         test_expect_success 'snapshots: give full hash' '
> 		ID=$(git rev-parse --verify HEAD) &&
> 		gitweb_run ...
>         '
> 
> Also, if I am not mistaken, "test_commit" is not about doing any test, but
> is a short-hand for doing an operation, right?  It would be better to have
> it inside test_expect_success just in case your "git commit" or some other
> commands are broken.  I.e. like
> 
> 	test_expect_success 'create a test commit' '
> 		test_commit SnapshotFileTests "Can I have shapshot?"
>         '

Can I have snapshot?!?! What do you have against LOLspeak? :P


-- 
Mark Rada (ferrous26)
marada@uwaterloo.ca

^ permalink raw reply

* Re: [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot
From: Junio C Hamano @ 2009-09-13  5:42 UTC (permalink / raw)
  To: Mark Rada; +Cc: Junio C Hamano, git, Jakub Narebski
In-Reply-To: <4AAC8521.1060005@mailservices.uwaterloo.ca>

Mark Rada <marada@uwaterloo.ca> writes:

> On 09-09-12 11:30 PM, Junio C Hamano wrote:
>>> @@ -5196,8 +5202,9 @@ sub git_snapshot {
>>>  		die_error(403, "Unsupported snapshot format");
>>>  	}
>>>  
>>> -	if (!defined $hash) {
>>> -		$hash = git_get_head_hash($project);
>>> +	my $full_hash = git_get_full_hash($project, $hash);
>>> +	if (!$full_hash) {
>>> +		die_error(404, 'Hash id was not valid');
>>>  	}
>> 
>> This is in the context of "snapshot", so obviously you care more about
>> just "such an object exists", don't you?  You also want it to be a
>> tree-ish.  Try giving it $hash = 'junio-gpg-pub' and see how it breaks.
>  
> You have confused me. How is using 'junio-gpg-pub' different from the 
> second test case that tries to use 'frizzumFrazzum'?

junio-gpg-pub tag exists in git.git but it tags a blob not a tree.

	$ git rev-parse junio-gpg-pub
        6019c27d966fe3ce8adcc0e9f12078eef96ca6ef
        $ git archive junio-gpg-pub
        fatal: not a tree object

^ permalink raw reply

* Re: [RFC/PATCH v4 2/2] gitweb: append short hash ids to snapshot files
From: Junio C Hamano @ 2009-09-13  5:57 UTC (permalink / raw)
  To: Mark Rada; +Cc: git, Jakub Narebski
In-Reply-To: <4AAC85AC.9080004@mailservices.uwaterloo.ca>

Mark Rada <marada@uwaterloo.ca> writes:

> This was a manifestation of a suggestion from Jakub:
>
>> Second, I'd rather have better names for snapshots than using full SHA-1.
>> For snapshot of 'v1.5.0' of repository 'repo.git' I'd prefer for snapshot
>> to be named 'repo-v1.5.0', and for snapshot of 'next' branch of the same

I think I've already said "Don't use $full_hash but if the user gave you
descriptive e.g. v1.5.0 in $hash just use it", which I think matches what
Jakub said above.

>> project to be named for example 'repo-next-20090909', or perhaps
>> 'repo-next-2009-09-10T09:16:18' or 'repo-next-20090909-g5f6b0ff',
>> or 'repo-v1.6.5-rc0-164-g5f6b0ff'.

I do not particularly care about these, except that if the user asked for
'next', that string should be in the name somewhere, so the last one is
unnacceptable to me.  I'd rather vote for naming it just 'repo-next', as
if I were writing a robot that goes once-a-day to next and download, I
would likely to be doing it like this:

	D=`date +'%Y-%m-%d'` && mkdir "$D" && cd "$D" || exit
        wget ...snapshot-url-for-next-branch...
	wget ...snapshot-url-for-some-other-branch...
	wget ...snapshot-url-for-even-some-other-repository...
	...

and I do not want any frills other than what I _asked_ gitweb to give me,
which is "this repository, this branch".

But that is just my personal preference.  Treat it as no heavier than a
feature request from a random list participant.  It does not carry any
more weight than that merely because it comes from me.

> For me, there are two fates that snapshots will end up with: being deleted
> as soon as I have unrolled the contents, or long term archiving. For the
> latter case, it is nice to have an idea of when it came from, though I
> guess I should have appended a date in that case... ¯\(°_o)/¯

What date do you mean?  The commit date?  Or download date?

As long as it is clear which revision the snapshot came from, I do not
think anything fancier is necessary.

Besides, don't the paths in the archive have the timestamp of the commit
object?

If you are talking about download date for archival use, I think the
timestamp of the archive file itself is sufficient, and the person who is
downloading can (re)name the result in whatever way he wants.

^ permalink raw reply

* Re: [PATCH] git-push: Accept -n as a synonym for --dry-run.
From: Junio C Hamano @ 2009-09-13  6:10 UTC (permalink / raw)
  To: Nelson Elhage; +Cc: Junio C Hamano, git
In-Reply-To: <7vd45v2za4.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> ..., so you are not arguing against
> me at all.
> ... it is likely that we would try very
> hard to find a name that does not begin with 'n' to avoid the issue.

So what's my objection, in short?

It is just the wording "_the_ standard" that implies that anything that
uses -n to mean something other than --dry-run is _wrong_.

I didn't want to see anybody arguing against "commit -n".  It is used to
defeat pre-commit hook and has been there with us for a long time.

^ permalink raw reply

* [PATCH] Improve --patch option documentation in git-add
From: Jari Aalto @ 2009-09-13  6:44 UTC (permalink / raw)
  To: git
In-Reply-To: <7vmy5fy2hz.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> Jari Aalto <jari.aalto@cante.net> writes:
>
>>     --patch:
>>     -p::
>>         In a modified work tree, choose interactively which patch hunks to
>>         add. This gives a change to review the difference between the
>>         index and the work before adding modified contents to the index.
>
> Sounds sensible.  You may want to be even more direct and succinct, e.g.
>
>     Interactively choose hunks of patch between the index and the work
>     tree and add them to the index.

Thanks, see below,
Jari

>From 63aa94e7782d6340ead0446ea80ed6223d7ac5c1 Mon Sep 17 00:00:00 2001
From: Jari Aalto <jari.aalto@cante.net>
Date: Sun, 13 Sep 2009 09:43:10 +0300
Subject: [PATCH] Improve --patch option documentation in git-add

Signed-off-by: Jari Aalto <jari.aalto@cante.net>
---
 Documentation/git-add.txt |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index e67b7e8..b94fbec 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -72,9 +72,14 @@ OPTIONS
 
 -p::
 --patch::
-	Similar to Interactive mode but the initial command loop is
-	bypassed and the 'patch' subcommand is invoked using each of
-	the specified filepatterns before exiting.
+	Interactively choose hunks of patch between the index and the
+	work tree and add them to the index. This gives a change to
+	review the difference before adding modified contents to the
+	index.
+
+	This effectively runs ``add --interactive``, but bypass the
+	initial command menu and directly jump to `patch` subcommand.
+	See ``Interactive mode'' for details.
 
 -e, \--edit::
 	Open the diff vs. the index in an editor and let the user
-- 
1.6.3.3

^ permalink raw reply related

* merge ignores --no-commit in fast-forward case
From: Tomas Carnecky @ 2009-09-13  6:40 UTC (permalink / raw)
  To: git mailing list

Bug or feature?

Three possible solutions that I see:

  2) Refuse to do anything if user gives --no-commit and merge is fast- 
forward
  3) Document this behavior in the manpage
  4) Quietly set deny_non_fast_forward when --no-commit was given

A fourth solution would be to not change anything, but users usually  
have a reason for using --no-commit, and if git commits behind their  
backs anyway that's just not nice.

tom

^ permalink raw reply

* Re: merge ignores --no-commit in fast-forward case
From: Junio C Hamano @ 2009-09-13  7:30 UTC (permalink / raw)
  To: Tomas Carnecky; +Cc: git mailing list
In-Reply-To: <A754ACF1-77C2-4036-A15C-8949E76BD2D5@dbservice.com>

Tomas Carnecky <tom@dbservice.com> writes:

> Bug or feature?

Feature, perhaps a misleading one.  As a fast-forward merge does not _create_
a commit, and --no-commit is about not creating a commit, it is logically
consistent.  But it is not useful nor intuitive to be logically consistent
in this case.

> Three possible solutions that I see:
>
>  2) Refuse to do anything if user gives --no-commit and merge is fast- 
> forward
>  3) Document this behavior in the manpage
>  4) Quietly set deny_non_fast_forward when --no-commit was given

Heh, this is new.  People laugh at me when I number my bullets starting
from zero, like all good computer scientists do ;-)

Seriously, we should at least do #3, and as a backward incompatible change
at least _consider_ doing #2 (I think #4 is merely an implementation detail
of #2), and if list reaches concensus in favor of such a change, come up
with a transition plan and do so in the 1.7.0 release.

^ permalink raw reply

* Re: [PATCH] use write_str_in_full helper to avoid literal string lengths
From: Jim Meyering @ 2009-09-13  7:32 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git list
In-Reply-To: <7veiqbbttg.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
...
> Thanks.  I agree with the reasoning you wrote outside the proposed log
> message.
>
> We usually do not write these bullet points (iow, we are not GNU) in our
> log message.  The names of the functions, call sites and files that are
> involved are something anybody can see from the patch text,
>
> I think the GNU convention was useful back when we were trapped in a
> system with non-atomic commits, where it was very hard to see what files
> were affected in a single logical changeset (i.e. CVS).
>
> Luckily, we graduated those dark ages.
>
> Instead, we prefer to have justifications (and methods), like what you
> wrote at the beginning of your message.  These are not something people
> can find in the patch text and they deserve to be recorded in the commit.

Heh ;-)
Old habits...
Here's that same commit with a better log message:

>From 7edbf0141c9e2ae2999f8b7919c938b9384240e2 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Sat, 12 Sep 2009 09:56:13 +0200
Subject: [PATCH] use write_str_in_full helper to avoid literal string lengths

In 2d14d65ce7b136b0fb18dcf27e5caff67829f658,
I noticed two changes like this:

-	write_in_full(helper->in, "list\n", 5);
+
+	strbuf_addstr(&buf, "list\n");
+	write_in_full(helper->in, buf.buf, buf.len);
+	strbuf_reset(&buf);

Instead, let's define a new function, in cache.h:

    static inline ssize_t write_str_in_full(int fd, const char *str)
    {
           return write_in_full(fd, str, strlen(str));
    }

and then use it like this:

-       strbuf_addstr(&buf, "list\n");
-       write_in_full(helper->in, buf.buf, buf.len);
-       strbuf_reset(&buf);
+       write_str_in_full(helper->in, "list\n");

Thus not requiring the added allocation, and still avoiding
the maintenance risk of literal string lengths.
These days, compilers are good enough that strlen("literal")
imposes no run-time cost.

Update the two uses in transport-helper.c manually,
and transform the other uses via this:

    perl -pi -e \
        's/write_in_full\((.*?), (".*?"), \d+\)/write_str_in_full($1, $2)/'\
      $(git grep -l 'write_in_full.*"')

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 builtin-fetch.c    |    2 +-
 builtin-reflog.c   |    2 +-
 cache.h            |    9 +++++++--
 commit.c           |    2 +-
 config.c           |    2 +-
 rerere.c           |    2 +-
 transport-helper.c |   10 +++-------
 upload-pack.c      |    4 ++--
 8 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 817dd6b..cb48c57 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -454,7 +454,7 @@ static int quickfetch(struct ref *ref_map)

 	for (ref = ref_map; ref; ref = ref->next) {
 		if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
-		    write_in_full(revlist.in, "\n", 1) < 0) {
+		    write_str_in_full(revlist.in, "\n") < 0) {
 			if (errno != EPIPE && errno != EINVAL)
 				error("failed write to rev-list: %s", strerror(errno));
 			err = -1;
diff --git a/builtin-reflog.c b/builtin-reflog.c
index 95198c5..e23b5ef 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c
@@ -362,7 +362,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,
 		} else if (cmd->updateref &&
 			(write_in_full(lock->lock_fd,
 				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-			 write_in_full(lock->lock_fd, "\n", 1) != 1 ||
+			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
 			 close_ref(lock) < 0)) {
 			status |= error("Couldn't write %s",
 				lock->lk->filename);
diff --git a/cache.h b/cache.h
index 30a7a16..90bf127 100644
--- a/cache.h
+++ b/cache.h
@@ -924,13 +924,18 @@ extern const char *git_mailmap_file;
 extern void maybe_flush_or_die(FILE *, const char *);
 extern int copy_fd(int ifd, int ofd);
 extern int copy_file(const char *dst, const char *src, int mode);
-extern ssize_t read_in_full(int fd, void *buf, size_t count);
-extern ssize_t write_in_full(int fd, const void *buf, size_t count);
 extern void write_or_die(int fd, const void *buf, size_t count);
 extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);
 extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);
 extern void fsync_or_die(int fd, const char *);

+extern ssize_t read_in_full(int fd, void *buf, size_t count);
+extern ssize_t write_in_full(int fd, const void *buf, size_t count);
+static inline ssize_t write_str_in_full(int fd, const char *str)
+{
+	return write_in_full(fd, str, strlen(str));
+}
+
 /* pager.c */
 extern void setup_pager(void);
 extern const char *pager_program;
diff --git a/commit.c b/commit.c
index a6c6f70..fedbd5e 100644
--- a/commit.c
+++ b/commit.c
@@ -212,7 +212,7 @@ int write_shallow_commits(int fd, int use_pack_protocol)
 			else {
 				if (write_in_full(fd, hex,  40) != 40)
 					break;
-				if (write_in_full(fd, "\n", 1) != 1)
+				if (write_str_in_full(fd, "\n") != 1)
 					break;
 			}
 		}
diff --git a/config.c b/config.c
index f21530c..c644061 100644
--- a/config.c
+++ b/config.c
@@ -1119,7 +1119,7 @@ int git_config_set_multivar(const char *key, const char *value,
 				    copy_end - copy_begin)
 					goto write_err_out;
 				if (new_line &&
-				    write_in_full(fd, "\n", 1) != 1)
+				    write_str_in_full(fd, "\n") != 1)
 					goto write_err_out;
 			}
 			copy_begin = store.offset[i];
diff --git a/rerere.c b/rerere.c
index 87360dc..29f95f6 100644
--- a/rerere.c
+++ b/rerere.c
@@ -61,7 +61,7 @@ static int write_rr(struct string_list *rr, int out_fd)
 		path = rr->items[i].string;
 		length = strlen(path) + 1;
 		if (write_in_full(out_fd, rr->items[i].util, 40) != 40 ||
-		    write_in_full(out_fd, "\t", 1) != 1 ||
+		    write_str_in_full(out_fd, "\t") != 1 ||
 		    write_in_full(out_fd, path, length) != length)
 			die("unable to write rerere record");
 	}
diff --git a/transport-helper.c b/transport-helper.c
index b1ea7e6..832d81f 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -37,9 +37,7 @@ static struct child_process *get_helper(struct transport *transport)
 		die("Unable to run helper: git %s", helper->argv[0]);
 	data->helper = helper;

-	strbuf_addstr(&buf, "capabilities\n");
-	write_in_full(helper->in, buf.buf, buf.len);
-	strbuf_reset(&buf);
+	write_str_in_full(helper->in, "capabilities\n");

 	file = fdopen(helper->out, "r");
 	while (1) {
@@ -58,7 +56,7 @@ static int disconnect_helper(struct transport *transport)
 {
 	struct helper_data *data = transport->data;
 	if (data->helper) {
-		write_in_full(data->helper->in, "\n", 1);
+		write_str_in_full(data->helper->in, "\n");
 		close(data->helper->in);
 		finish_command(data->helper);
 		free((char *)data->helper->argv[0]);
@@ -124,9 +122,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)

 	helper = get_helper(transport);

-	strbuf_addstr(&buf, "list\n");
-	write_in_full(helper->in, buf.buf, buf.len);
-	strbuf_reset(&buf);
+	write_str_in_full(helper->in, "list\n");

 	file = fdopen(helper->out, "r");
 	while (1) {
diff --git a/upload-pack.c b/upload-pack.c
index c77ab71..b3471e4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -553,7 +553,7 @@ static void receive_needs(void)

 	shallow_nr = 0;
 	if (debug_fd)
-		write_in_full(debug_fd, "#S\n", 3);
+		write_str_in_full(debug_fd, "#S\n");
 	for (;;) {
 		struct object *o;
 		unsigned char sha1_buf[20];
@@ -619,7 +619,7 @@ static void receive_needs(void)
 		}
 	}
 	if (debug_fd)
-		write_in_full(debug_fd, "#E\n", 3);
+		write_str_in_full(debug_fd, "#E\n");

 	if (!use_sideband && daemon_mode)
 		no_progress = 1;
--
1.6.5.rc0.190.g15871

^ permalink raw reply related

* Re: [PATCH] transport-helper.c: don't leak fdopen'd stream buffers
From: Jim Meyering @ 2009-09-13  7:45 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git list, Daniel Barkalow, Johannes Sixt
In-Reply-To: <7vtyz760lm.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> diff --git a/transport-helper.c b/transport-helper.c
>> index f57e84c..0bbd014 100644
>> --- a/transport-helper.c
>> +++ b/transport-helper.c
>> @@ -49,6 +49,7 @@ static struct child_process *get_helper(struct transport *transport)
>>  		if (!strcmp(buf.buf, "fetch"))
>>  			data->fetch = 1;
>>  	}
>> +	fclose (file);
>>  	return data->helper;
>>  }
>>
>> @@ -88,6 +89,7 @@ static int fetch_with_fetch(struct transport *transport,
>>  		if (strbuf_getline(&buf, file, '\n') == EOF)
>>  			exit(128); /* child died, message supplied already */
>>  	}
>> +	fclose (file);
>>  	return 0;
>>  }
>
> The callchain of fetch_with_fetch() looks like:
>
>     fetch_with_fetch()
>         helper = get_helper();
>         --> get_helper()
>             - start helper with start_command();
>             - read from helper->out until it sees an empty line;
>             - break out of the loop;
>         <-- return helper
>         - file = xfdopen(helper->out) to get another FILE on the fd
>         - read the rest of the output from helper->out via file
>
> It seems to me that the fclose() in get_helper() will close the underlying
> fd and would break the caller, no?

I confess that my sole test was to run "make test", which passed.
If the fd must live on, a slightly less invasive change would be to
xdup each descriptor just before each of the three xfdopen calls, e.g.,

-       file = xfdopen(helper->out, "r");
+       file = xfdopen(xdup(helper->out), "r");

> I think "struct helper_data" should get a new FILE* field and once
> somebody creates a FILE* out of its helper->out, that FILE* can be passed
> around without a new xfdopen().
>
> Or something like that.

That's probably best.

> Who is responsible for closing the underlying helper->out fd in the
> start_command() API, by the way?

^ permalink raw reply

* Re: Effectively tracing project contributions with git
From: Jeff King @ 2009-09-13  9:24 UTC (permalink / raw)
  To: Theodore Tso; +Cc: Joseph Wakeling, Sverre Rabbelier, git
In-Reply-To: <20090913022843.GB26588@mit.edu>

On Sat, Sep 12, 2009 at 10:28:43PM -0400, Theodore Tso wrote:

> > I don't see any solution that doesn't see me browsing diffs -- there's
> > no metric that will solve the problem -- but if your stats work could
> > help me get an output of the form 'here are all the diffs on file X by
> > contributor Y in order of size, largest first' then I think it would
> > help a LOT.
> 
> This will display all of the diffs on file (pathname) XXX by contributor YYY:
> 
> 	git log -p --author=YYY XXX 
> 
> You might also find the diffstats useful:
> 
> 	git log --stat --author=YYY XXX
> 
> Or if you want *only* the diffstats for the file in question, you might try:
> 
> 	git log --stat --pretty=format: --author=YYY XXX | grep XXX

There is also the "--numstat" format which is a bit easier for parsing.
I think the "all diffs on file $X by contributor $Y, ordered by size"
would look like:

  git log -z --pretty=tformat:%H --numstat --author=$Y $X |
  perl -0ne '
    my ($commit) = /^([0-9a-f]{40})$/m;
    my ($lines_added) = /^(\d+)\s/m;
    print "$lines_added $commit\n";
  ' |
  sort -rn |
  cut -d ' ' -f2 |
  xargs -n 1 git show

-Peff

^ permalink raw reply

* Re: git push --confirm ?
From: Jeff King @ 2009-09-13  9:33 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Owen Taylor, git, Colin Walters
In-Reply-To: <7vvdjn8ymk.fsf@alter.siamese.dyndns.org>

On Sat, Sep 12, 2009 at 05:41:23PM -0700, Junio C Hamano wrote:

> > But what _would_ be useful is doing it atomically. You can certainly do
> > all three of those steps from within one "git push" invocation, and I
> > think that is enough without any protocol changes. The protocol already
> > sends for each ref a line like:
> >
> >   <old-sha1> <new-sha1> <ref>
> >
> > and receive-pack will not proceed with the update unless the <old-sha1>
> > matches what is about to be changed.
> 
> Be careful that using that information and doing things in one session
> won't give you atomicity in the sense that it may still fail after you
> said "yes that is what I want to push, really" to the confirmation
> question.

Of course, but that issue exists already. It is just that the window
between receiving the refs and then asking them to be updated is much
smaller when there is no human input in the loop (and since we haven't
actually _shown_ the list to the user, it appears atomic to them).

I think this type of atomicity is fine for this application. The point
of this is to err on the side of caution. So it is OK to say "Push
this?" and then after the user has confirmed say "Oops, somebody pushed
something else while we were waiting for your input. Try again." The
important thing is to not say "Push this?", have the user confirm that
what they are pushing over is OK, and then end up pushing over something
different (which is what can happen with separate push invocations).

The only way to get true atomicity across the confirmation and push
would be to take a lock at the beginning of the push session. Which is
too coarse-grained in the first place (it disallows simultaneous update
of unrelated refs), but would also require protocol updates.

> It does save you an extra connection, compared to separate invocations
> without and then with --dry-run, so it still is a plus.
> 
> I do not think this is an unreasonable option to have.  Just please don't
> justify this change based on atomicity argument, but justify it as a mere
> convenience feature.

I don't agree. Making sure we use the _same_ <old-sha1> in the
confirmation output we show to the user and in the ref update we send to
the remote is critical for this to be safe.

-Peff

^ permalink raw reply

* Re: [PATCH] reflog documentation: -n is an alias for --dry-run
From: Jeff King @ 2009-09-13  9:40 UTC (permalink / raw)
  To: Nelson Elhage; +Cc: git
In-Reply-To: <1252813314-14408-1-git-send-email-nelhage@mit.edu>

On Sat, Sep 12, 2009 at 11:41:54PM -0400, Nelson Elhage wrote:

>  static const char reflog_expire_usage[] =
> -"git reflog (show|expire) [--verbose] [--dry-run] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...";
> +"git reflog (show|expire) [--verbose] [-n | --dry-run] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...";

Really? I think "git reflog show -n" is not about dry-run at all...

-Peff

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox