On Tue, Mar 20, 2012 at 12:31:33AM -0400, Christopher Tiwald wrote:
> Changes since v2:
> - Cleaned up commit message language, specifically in scenario
> one.
> - Created one config variable per piece of non-ff push advice.
> Additionally, preserved 'pushNonFastForward' as a means of
> disabling all non-ff push advice. Users who set this
> config option should see no change to 'git push'.
This version looks pretty good to me, but I have one question:
> @@ -1008,7 +1009,6 @@ struct ref {
> char *symref;
> unsigned int force:1,
> merge:1,
> - nonfastforward:1,
> deletion:1;
> enum {
> REF_STATUS_NONE = 0,
> @@ -1019,6 +1019,10 @@ struct ref {
> REF_STATUS_REMOTE_REJECT,
> REF_STATUS_EXPECTING_REPORT
> } status;
> + enum {
> + NON_FF_HEAD = 1,
> + NON_FF_OTHER
> + } nonfastforward;
> char *remote_status;
> struct ref *peer_ref; /* when renaming */
> char name[FLEX_ARRAY]; /* more */
Why is this enum stored inside the ref? It doesn't actually know whether
it is a HEAD or not, and we never actually store that value there. We
always just store a boolean (remote.c, ll. 1294-1298) and access it as
one (remote.c, l. 1300; transport.c, l. 1259).
The only time we use the enum values is via the "int nonfastforward"
passed to transport_push. I think it would be a lot clearer to leave
nonfastforward as a single bit in the ref, and then define the enum
elsewhere (or even just use #define if we are not going to use the enum
type). Like this on top of your patch:
diff --git a/cache.h b/cache.h
index 427b600..35f3075 100644
--- a/cache.h
+++ b/cache.h
@@ -1009,6 +1009,7 @@ struct ref {
char *symref;
unsigned int force:1,
merge:1,
+ nonfastforward:1,
deletion:1;
enum {
REF_STATUS_NONE = 0,
@@ -1019,10 +1020,6 @@ struct ref {
REF_STATUS_REMOTE_REJECT,
REF_STATUS_EXPECTING_REPORT
} status;
- enum {
- NON_FF_HEAD = 1,
- NON_FF_OTHER
- } nonfastforward;
char *remote_status;
struct ref *peer_ref; /* when renaming */
char name[FLEX_ARRAY]; /* more */
diff --git a/transport.h b/transport.h
index ce99ef8..1631a35 100644
--- a/transport.h
+++ b/transport.h
@@ -138,6 +138,8 @@ int transport_set_option(struct transport *transport, const char *name,
void transport_set_verbosity(struct transport *transport, int verbosity,
int force_progress);
+#define NON_FF_HEAD 1
+#define NON_FF_OTHER 2
int transport_push(struct transport *connection,
int refspec_nr, const char **refspec, int flags,
int * nonfastforward);
I don't think your patch is buggy, because the enum is perfectly capable
of being used as a single bit. But it's confusing to read, because
ref->nonfastforward will never actually be set to the NON_FF_OTHER enum
value.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html